impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5570: fix spilling null-aware anti join
Date Wed, 23 Aug 2017 21:56:57 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message