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-5788: Fix agg node crash when grouping by nondeterministic exprs
Date Fri, 18 Aug 2017 21:29:26 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs
......................................................................


Patch Set 2:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/7714/2/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

Line 1160:   // partition index
nit: period at end of sentence for consistency.


PS2, Line 1163: hashTable
hash_table. Although I don't think we really need this temporary variable in the first place.


Line 1407:   // We might be dealing with a rebuilt spilled partition, where all partitions
are
Maybe add something to the start of this to explain the broader purpose (otherwise it sounds
like the code is only dealing with this corner case). Maybe something like "Remove references
to the destroyed hash table from 'hash_tbls_'.


Line 1409:   // with it as well
nit: end with period for consistency.


Line 1426:     // right partition
nit: end with period for consistency.


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

Line 352: group by 1,2,3,4,5, random()
nit: add spaces after commas for consistency (this was from my original sloppy formatting).


Line 354: ---- RUNTIME_PROFILE
It would be nice to have a RESULTS section to verify that the query isn't returning anything
crazy, but I can't think of a good way to both have this non-deterministic behaviour and return
consistent results.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message