You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2017/08/17 22:53:13 UTC

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Bikramjeet Vig has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7714

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

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

Fixed a bug where impala crashes during execution of an aggregation
query using nondeterministic grouping expressions. This happens when
it tries to rebuild a spilled partition that can fit in memory and rows
get re-hashed to a partition other than the spilled one due to the use
of nondeterministic expressions.

Testing:
Added a query test to verify successful execution.

Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
---
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
3 files changed, 44 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7714/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(6 comments)

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

PS1, Line 1389: isRebuiltSpilledPartition
wrong casing for vars in C++


Line 1419:     for (int i = 0; i < PARTITION_FANOUT; ++i) hash_tbls_[i] = NULL;
How about something like this that works for both cases:

  for (int i = 0; i < PARTITION_FANOUT; ++i) {
    if (hash_partitions_[i] == hash_partitions_[partition_idx]) hash_tbls_[i] = nullptr;
  }

This avoids the need for the isRebuiltSpilledPartition logic.


Line 1431:   for (int i = 0; i < hash_partitions_.size(); ++i) {
I think we need to check other places that iterate over hash_partitions_ to make sure they handle duplicate partitions correctly. E.g. this function doesn't do the right thing. I don't know if it just works by coincidence or what.


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

Line 337:   /// Current partitions we are partitioning into.
Can you update the comment for hash_partitions_ too to explain the two cases (distinct partitions versus all pointing to the same one).


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

Line 349: set default_spillable_buffer_size=64k;
I don't think this query option will take effect, since it's set by the test_spilling.py code. This is a weird glitch in the test infra, but what happens is that these options are set in the session on the Impala daemon, then they're overridden by the query options sent along with the query.

We should remove this "set" and confirm that the test reproduces the crash with the alternative buffer size.


PS1, Line 354: ;
Don't need trailing semicolon.


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

Bikramjeet Vig has posted comments on this change.

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


Patch Set 1:

(6 comments)

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

PS1, Line 1389: isRebuiltSpilledPartition
> wrong casing for vars in C++
removed as a part of you next comment


Line 1419:     for (int i = 0; i < PARTITION_FANOUT; ++i) hash_tbls_[i] = NULL;
> How about something like this that works for both cases:
Done


Line 1431:   for (int i = 0; i < hash_partitions_.size(); ++i) {
> I think we need to check other places that iterate over hash_partitions_ to
Thanks for pointing this out, I checked and we only iterate over hash_partitions_ in LargestSpilledPartition(), SpillPartition() and MoveHashPartitions().
The first 2 are harmless as they only look for the largest partitions in the loop, but that last one as you pointed out does not do the right thing. Hence added a condition for that check.


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

Line 337:   /// Current partitions we are partitioning into.
> Can you update the comment for hash_partitions_ too to explain the two case
Done


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

Line 349: set default_spillable_buffer_size=64k;
> I don't think this query option will take effect, since it's set by the tes
confirmed. Test crashes on master branch with the alternative buffer size


PS1, Line 354: ;
> Don't need trailing semicolon.
Done


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

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


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1122/

-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

Bikramjeet Vig has posted comments on this change.

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


Patch Set 3:

(1 comment)

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

Line 1160:   // partition index.
> Repartitioning should work ok in that each repartitioning step will reduce 
Just to add to this, we would also have to make the same effort for hash join node to stay consistent


-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

Bikramjeet Vig has posted comments on this change.

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


Patch Set 2:

(8 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.
Done


PS2, Line 1163: hashTable
> hash_table. Although I don't think we really need this temporary variable i
Done


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 (ot
Done


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


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


PS2, Line 1487:     AggFnEvaluator::FreeLocalAllocations(partition->agg_fn_evals);
Just want to make sure:
1. Will this method do the wrong thing if it runs multiple times on the same partition for our rebuilt spilled partition case?

2. Also, AggFnEvaluator::FreeLocalAllocations executes a lot of code, so will a check like "if(i != partition->idx) continue;" to avoid rerunning it on the same partition help with perf?


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 slo
Done


Line 354: ---- RUNTIME_PROFILE
> It would be nice to have a RESULTS section to verify that the query isn't r
Exactly. There is no way to check for consistent results, so I just put a check to see if the query runs to completion. Also, is there any other profile stat you think I should add to the check?


-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

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


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1127/

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

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


Patch Set 3:

(1 comment)

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

Line 1160:   // partition index.
> Yes, in case of repartition, the rows do get shuffled around in each iterat
Repartitioning should work ok in that each repartitioning step will reduce the size of partitions - each row ends up in an arbitrary partition. The end result is pretty much arbitrary though.

I thought this was a reasonable solution. I would be ok with a solution that failed queries with nondeterministic grouping functions but I couldn't think of another simpler solution that met the following two constraints:
* Guarantees that a nondeterministic UDF or builtin can't crash Impala
* Doesn't impose overhead on the "fast path".

Runtime checks for whether the row mapped to the right partition would impose some runtime overhead. We could avoid that by codegen'ing a different version of the probe function for the case when we're processing a single spilled partition, but then that would add codegen overhead.


-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change.

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


Patch Set 4: Verified+1

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

Bikramjeet Vig has uploaded a new patch set (#2).

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

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

Fixed a bug where impala crashes during execution of an aggregation
query using nondeterministic grouping expressions. This happens when
it tries to rebuild a spilled partition that can fit in memory and rows
get re-hashed to a partition other than the spilled one due to the use
of nondeterministic expressions.

Testing:
Added a query test to verify successful execution.

Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
---
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
3 files changed, 43 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7714/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

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

Patch Set 3:

(2 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 1160: // partition index.
> Just to add to this, we would also have to make the same effort for hash jo
i agree it's best to not adding a check to the fast path. We can go forward with this fix, but please do add that comment referencing this jira to the header file.

Why isn't this a problem in the join? Is it because we repartition into the stream first, and then build hashtables over a single stream in the join case? whereas in agg we're repartitioning and building the table in the same pass?

Ultimately I think we should eventually reject these queries during analysis (e.g. IMPALA-4605), but that's obviously out of scope for now.

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
> You are absolutely right. This is very specific to our bug fix. would it ma
I think adding a reference to the JIRA makes sense in this case since it's so specific to that.

--
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

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


Patch Set 3: Code-Review+2

-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

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


Patch Set 3:

I think dan should have a look too

-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

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

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

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


Patch Set 2:

(2 comments)

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

PS2, Line 1487:     AggFnEvaluator::FreeLocalAllocations(partition->agg_fn_evals);
> Just want to make sure:
It's idempotent, so should be fine (after it runs the first time there are no local allocations).

I wouldn't worry about the perf. We already do this on 16 partitions on the more perf-critical non-spilling path, so this is no worse. I think if we wanted to optimise this, we'd focus on that case first. I'd prefer to keep the code simpler.


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 354: ---- RUNTIME_PROFILE
> Exactly. There is no way to check for consistent results, so I just put a c
The ones you have seem sufficient.


-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

Dan Hecht has posted comments on this change.

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


Patch Set 4: Code-Review+2

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged.

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


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

Fixed a bug where impala crashes during execution of an aggregation
query using nondeterministic grouping expressions. This happens when
it tries to rebuild a spilled partition that can fit in memory and rows
get re-hashed to a partition other than the spilled one due to the use
of nondeterministic expressions.

Testing:
Added a query test to verify successful execution.

Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
Reviewed-on: http://gerrit.cloudera.org:8080/7714
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
3 files changed, 42 insertions(+), 5 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

Bikramjeet Vig has uploaded a new patch set (#3).

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

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

Fixed a bug where impala crashes during execution of an aggregation
query using nondeterministic grouping expressions. This happens when
it tries to rebuild a spilled partition that can fit in memory and rows
get re-hashed to a partition other than the spilled one due to the use
of nondeterministic expressions.

Testing:
Added a query test to verify successful execution.

Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
---
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
3 files changed, 43 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7714/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change.

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


Patch Set 3: Code-Review+1

-- 
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5788: Fix agg node crash when grouping by nondeterministic exprs

Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.

Hello Tim Armstrong, Dan Hecht,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7714

to look at the new patch set (#4).

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

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

Fixed a bug where impala crashes during execution of an aggregation
query using nondeterministic grouping expressions. This happens when
it tries to rebuild a spilled partition that can fit in memory and rows
get re-hashed to a partition other than the spilled one due to the use
of nondeterministic expressions.

Testing:
Added a query test to verify successful execution.

Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
---
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-aggregation-node.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
3 files changed, 42 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/7714/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7714
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibdb09239577b3f0a19d710b0d148e882b0b73e23
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>