Dan Hecht has posted comments on this change. Change subject: IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/7714/3/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 1156: nit: to get more code on the screen, i think we can do without blank lines where the code is so closely related. Line 1160: // partition index. in the case where we do repartition (and prior to IMPALA-2708), what happens? the row gets shuffled around random partitions in each iteration? do things actually work in any meaningful way? I see how doing this avoids the crash, but have we thought about just giving an error when the grouping expression is non-deterministic? do we think that breaks important queries and that they had worked in meaningful ways? http://gerrit.cloudera.org:8080/#/c/7714/3/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: PS3, Line 338: all pointers in this vector will point to a single : /// in-memory partition outside of the context of this bug fix, it's hard to see why things would be set up that way. Is there some logical way we can justify it? Line 344: /// hash table part of a single in-memory partition same -- To view, visit http://gerrit.cloudera.org:8080/7714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
