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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
