You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/07/06 22:17:03 UTC

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7367

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and note trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 407 insertions(+), 85 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 13: Code-Review+2

carry

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 12: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7367/12/tests/query_test/test_spilling.py
File tests/query_test/test_spilling.py:

Line 66:     and therefore will not reliably pass with the deny reservation debug action enabled.
do we plan to fix that (and a jira) or is this case not important enough?


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7367/1//COMMIT_MSG
Commit Message:

PS1, Line 13: note
not


Line 25:   Instead we just iterate over the rows of the stream.
> are there join class comments that should be updated to explain this strate
I updated the member declarations to mentioned when things are pinned/unpinned and updated the EvaluateNullProbe() comment. I think this covers all of the points in the commit message.


http://gerrit.cloudera.org:8080/#/c/7367/1/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

Line 251:     RETURN_IF_ERROR(null_aware_partition_->Spill(BufferedTupleStreamV2::UNPIN_ALL));
> it's a bit confusing that we call Partition::Spill() when Partition::is_spi
Improved the comment.


http://gerrit.cloudera.org:8080/#/c/7367/1/be/src/exec/partitioned-hash-join-node.h
File be/src/exec/partitioned-hash-join-node.h:

PS1, Line 95: /// Null aware anti-join (NAAJ) extends the above algorithm by accumulating rows with
            : /// NULLs into several different streams, which are processed in a separate step to
            : /// produce additional output rows. The NAAJ algorithm is documented in more detail in
            : /// header comments for the null aware functions and data structures.
> any of this need updates (or the comments this references)?
Done.

It would be nice to have a clearer explanation of the NAAJ here (i.e. why NULLs should be treated that wasy) but that seems out of scope for this change.


http://gerrit.cloudera.org:8080/#/c/7367/1/testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
File testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test:

Line 3: set max_block_mgr_memory=10m;
Moved below comment


Line 8: # This returns the rows returned from as
garbled


Line 17: where l_suppkey = 4162 and l_shipmode = 'AIR' and l_returnflag = 'A' and l_shipdate > '1993-01-01' and
Fixed long lines in files


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 13: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#5).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 422 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#11).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
partitions.
* Probe streams are unpinned whenever there is memory pressure - if
spilling is enabled and either a build partition is spilled or
appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj-no-deny-reservation.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
8 files changed, 435 insertions(+), 91 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/11
--
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#2).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 425 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 12: Code-Review+2

rebase

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#4).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 422 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#8).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 422 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 7:

PS7 is a rebase

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 8:

Rebase onto the BTSV2 -> BTS rename

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7367/1//COMMIT_MSG
Commit Message:

PS1, Line 13: note
not


Line 25:   Instead we just iterate over the rows of the stream.
are there join class comments that should be updated to explain this strategy?


http://gerrit.cloudera.org:8080/#/c/7367/1/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

Line 251:     RETURN_IF_ERROR(null_aware_partition_->Spill(BufferedTupleStreamV2::UNPIN_ALL));
it's a bit confusing that we call Partition::Spill() when Partition::is_spilled() is already true. The comment here helps, but maybe something we can say in the Spill() function comment about this use of Spill()? do we do this elsewhere?


http://gerrit.cloudera.org:8080/#/c/7367/1/be/src/exec/partitioned-hash-join-node.h
File be/src/exec/partitioned-hash-join-node.h:

PS1, Line 95: /// Null aware anti-join (NAAJ) extends the above algorithm by accumulating rows with
            : /// NULLs into several different streams, which are processed in a separate step to
            : /// produce additional output rows. The NAAJ algorithm is documented in more detail in
            : /// header comments for the null aware functions and data structures.
any of this need updates (or the comments this references)?


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7367/10//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4674: Part 3: fix null-aware anti join
> This now has its own JIRA: IMPALA-5570
Done


http://gerrit.cloudera.org:8080/#/c/7367/10/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

PS10, Line 330: int i = 0; i < PARTITION_FANOUT; ++i)
> doesn't look like you need 'i' anymore so could make this range based
Done


http://gerrit.cloudera.org:8080/#/c/7367/10/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

PS10, Line 205: frees all reservation
> does it free the reservation or the buffers?  doesn't it hold on to the res
yeah reworded to make it a bit clearer what it's doing.


http://gerrit.cloudera.org:8080/#/c/7367/10/be/src/exec/partitioned-hash-join-node-ir.cc
File be/src/exec/partitioned-hash-join-node-ir.cc:

PS10, Line 440: AppendSpilledProbeRow
> why add this rather than just always using the new AppendProbeRow() (maybe 
At most of the callsites the stream will never be pinned. I don't think having the fallback logic to unpin the stream makes sense at those callsites. This seems a bit more streamlined and makes the intent at the callsite a bit clearer (I think).


http://gerrit.cloudera.org:8080/#/c/7367/10/tests/query_test/test_spilling.py
File tests/query_test/test_spilling.py:

Line 64:     """Null-aware anti-join tests that will not pass with the debug action enabled"""
> why don't they work?
Added a bit more explanation.


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#8).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 422 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 13: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1142/

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 13:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1144/

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7367/10//COMMIT_MSG
Commit Message:

Line 7: IMPALA-4674: Part 3: fix null-aware anti join
This now has its own JIRA: IMPALA-5570


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#7).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 422 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 13:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1142/

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 4:

Rebased

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7367/12/tests/query_test/test_spilling.py
File tests/query_test/test_spilling.py:

Line 66:     and therefore will not reliably pass with the deny reservation debug action enabled.
> do we plan to fix that (and a jira) or is this case not important enough?
I guess there are two reasons: one are the tests where scratch_limit=0, where that's just the nature of the test.

The second is the build side limitation. I filed IMPALA-5834 for that. It's unclear how important it is but it seems worth noting. There's a similar issue if there's a large number of duplicate values as well. I think both boil down to doing a spilling nested-loop join.


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7367/10/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

PS10, Line 330: int i = 0; i < PARTITION_FANOUT; ++i)
doesn't look like you need 'i' anymore so could make this range based


http://gerrit.cloudera.org:8080/#/c/7367/10/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

PS10, Line 205: frees all reservation
does it free the reservation or the buffers?  doesn't it hold on to the reservation but just free up the use of the reservation (i.e. the buffers)?


http://gerrit.cloudera.org:8080/#/c/7367/10/be/src/exec/partitioned-hash-join-node-ir.cc
File be/src/exec/partitioned-hash-join-node-ir.cc:

PS10, Line 440: AppendSpilledProbeRow
why add this rather than just always using the new AppendProbeRow() (maybe with a dcheck that we only end up on the slow path if not spilled)?


http://gerrit.cloudera.org:8080/#/c/7367/10/tests/query_test/test_spilling.py
File tests/query_test/test_spilling.py:

Line 64:     """Null-aware anti-join tests that will not pass with the debug action enabled"""
why don't they work?


-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................


IMPALA-5570: fix spilling null-aware anti join

IMPALA-4672: Part 2 regressed NAAJ by tightening up the spilling
invariants (e.g.  can't unpin with spilling disabled) but we
didn't have tests for spilling NAAJs that could detect the
regression. This patch adds those tests, fixes the regressions,
and improves NAAJ by reliably spilling the probe side and not
trying to bring the whole probe side into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Reviewed-on: http://gerrit.cloudera.org:8080/7367
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj-no-deny-reservation.test
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
8 files changed, 435 insertions(+), 91 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 9:

Rebased onto master. I started hitting IMPALA-5815 when running tests locally but it's unrelated to this change.

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................


Patch Set 10:

The rebase required splitting out the tests into two categories: those that can handle the debug action and those that can't.

-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#9).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Part 2 regressed NAAJ by tightening up the spilling invariants (e.g.
can't unpin with spilling disabled) but we didn't have tests for
spilling NAAJs that could detect the regression. This patch adds
those tests, fixes the regressions, and improves NAAJ by reliably
spilling the probe side and not trying to bring the whole probe side
into memory.

The changes are:
* All null-aware streams start off in memory and are only unpinned if
  spilling is enabled.
* The null-aware build partition can be spilled in the same way as hash
  partitions.
* Probe streams are unpinned whenever there is memory pressure - if
  spilling is enabled and either a build partition is spilled or
  appending to the probe stream fails.
* Spilled probe streams are not re-pinned in EvaluateNullProbe().
  Instead we just iterate over the rows of the stream.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
---
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
A testdata/workloads/functional-query/queries/QueryTest/spilling-naaj.test
M tests/query_test/test_spilling.py
7 files changed, 422 insertions(+), 91 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#12).

Change subject: IMPALA-5570: fix spilling null-aware anti join
......................................................................

IMPALA-5570: fix spilling null-aware anti join

IMPALA-4672: Part 2 regressed NAAJ by tightening up the spilling
invariants (e.g. can't unpin with spilling disabled) but we
didn't have tests for spilling NAAJs that could detect the
regression. This patch adds those tests, fixes the regressions,
and improves NAAJ by reliably spilling the probe side and not
trying to bring the whole probe side into memory.

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/12
--
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4674: Part 3: fix null-aware anti join

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has uploaded a new patch set (#10).

Change subject: IMPALA-4674: Part 3: fix null-aware anti join
......................................................................

IMPALA-4674: Part 3: fix null-aware anti join

Testing:
Add query tests where the three different buckets of rows are large
enough to spill: the build and probe of the null-aware partition and the
null probe rows.

Test both spilling and in-memory (with spilling disabled) cases.

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/67/7367/10
--
To view, visit http://gerrit.cloudera.org:8080/7367
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie2e60eb4dd32bd287a31479a6232400df65964c1
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>