impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs
Date Tue, 22 Aug 2017 19:54:00 GMT
Bikramjeet Vig has posted comments on this change.

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


Patch Set 3:

(3 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 
will do in the next patch set.
Thanks


Line 1160:   // partition index.
> in the case where we do repartition (and prior to IMPALA-2708), what happen
Yes, in case of repartition, the rows do get shuffled around in each iteration. I think the
non-deterministic nature of the group by exprs justify this behavior. Although it makes it
harder to be specific about what the meaningful way is.

I discussed this with Tim, and he pointed out that it might become prohibitive to figure out
if the query is non-deterministic, specially in case a non-deterministic UDF is used as a
group by expr. We can also put a check to identify if the rows hash to a different partition
on each iteration, but that would add code to the hot path and doesn't seem worth it.

For the last part of your question, I am not aware of any queries that do this, but this special
case might very well exist, if not intentionally, in someone's workload. This way we ensure
it continues to behave the same.

- Request Tim to pitch in if I missed something. Thanks


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 b
You are absolutely right. This is very specific to our bug fix. would it make sense if I add
the Jira ID in the comment? This would be able to provide more context in case some one trying
to understand this part needs it.


-- 
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 <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message