You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2024/12/18 19:31:17 UTC

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22235


Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing PlanNode.smallestCardinality()
function that correctly compares two cardinality numbers and return the
smallest and valid one.

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
3 files changed, 541 insertions(+), 32 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 8: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11228/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 01:01:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1417
PS2, Line 1417: 
> This function name does not fully describe what the function does.  I think
Done


http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1417
PS2, Line 1417: }
> Please write junit tests for this new function.
Added testSmallestValidCardinality.


http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1423
PS2, Line 1423: 
> Why is this function declared here instead of in AggregationNode?  It's onl
I think I hit this same issue somewhere else before. It is general enough to live inside PlanNode.
ps3 replace capCardinalityAtLimit() calls with smallestValidCardinality().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 19:47:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11255/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 30 Dec 2024 15:51:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 20:47:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17746/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 19:07:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1417
PS2, Line 1417:   protected static long smallestCardinality(long cardinality1, long cardinality2) {
Please write junit tests for this new function.


http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1417
PS2, Line 1417: smallestCardinality
This function name does not fully describe what the function does.  I think a name such as "smallestValidCardinality" would be more descriptive.


http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1423
PS2, Line 1423:   }
Why is this function declared here instead of in AggregationNode?  It's only used in AggregationNode.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 18:32:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 6: Code-Review+2

(2 comments)

looks good, only nits

http://gerrit.cloudera.org:8080/#/c/22235/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/22235/6//COMMIT_MSG@18
PS6, Line 18: generalize and replace
nit: generalizes/replaces


http://gerrit.cloudera.org:8080/#/c/22235/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@482
PS6, Line 482:     // and aggInputCardinality unknown.
nit: missing "is"



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 19:15:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17751/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 02:05:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11257/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 02:04:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestValidCardinality() and
MathUtil.saturatingMultiplyCardinalities(). Both function validates that
the function arguments are valid cardinality number.
smallestValidCardinality() correctly compares two cardinality numbers
and return the smallest and valid one. It generalizes and replaces
static function PlanNode.capCardinalityAtLimit().
saturatingMultiplyCardinalities() adds validation and normalization over
MathUtil.saturatingMultiply().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added more preconditions in AgggregationNode.java.
- Added CardinalityTest.testSmallestValidCardinality and
  MathUtilTest.testSaturatingMultiplyCardinality.
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Reviewed-on: http://gerrit.cloudera.org:8080/22235
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Jason Fehr <jf...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/util/MathUtil.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/util/MathUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
7 files changed, 614 insertions(+), 44 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified
  Jason Fehr: Looks good to me, but someone else must approve

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 3: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1417
PS2, Line 1417: 
> Done
Done


http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1417
PS2, Line 1417: }
> Added testSmallestValidCardinality.
Done

Nice unit test!


http://gerrit.cloudera.org:8080/#/c/22235/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1423
PS2, Line 1423: 
> I think I hit this same issue somewhere else before. It is general enough t
Makes sense to me.  Thanks!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 20:40:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Jason Fehr, Pranav Lodha, Michael Smith, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestValidCardinality() and
MathUtil.saturatingMultiplyCardinalities(). Both function validates that
the function arguments are valid cardinality number.
smallestValidCardinality() correctly compares two cardinality numbers
and return the smallest and valid one. It generalizes and replaces
static function PlanNode.capCardinalityAtLimit().
saturatingMultiplyCardinalities() adds validation and normalization over
MathUtil.saturatingMultiply().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added more preconditions in AgggregationNode.java.
- Added CardinalityTest.testSmallestValidCardinality and
  MathUtilTest.testSaturatingMultiplyCardinality.
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/util/MathUtil.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/util/MathUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
7 files changed, 617 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/22235/7
-- 
To view, visit http://gerrit.cloudera.org:8080/22235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 19:47:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has removed a vote on this change.

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
-- 
To view, visit http://gerrit.cloudera.org:8080/22235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11255/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 30 Dec 2024 21:27:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17726/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 19:58:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Jason Fehr, Pranav Lodha, Michael Smith, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestValidCardinality() and
MathUtil.saturatingMultiplyCardinalities(). Both function validates that
the function arguments are valid cardinality number.
smallestValidCardinality() correctly compares two cardinality numbers
and return the smallest and valid one. It generalizes and replaces
static function PlanNode.capCardinalityAtLimit().
saturatingMultiplyCardinalities() adds validation and normalization over
MathUtil.saturatingMultiply().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added more preconditions in AgggregationNode.java.
- Added CardinalityTest.testSmallestValidCardinality and
  MathUtilTest.testSaturatingMultiplyCardinality.
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/util/MathUtil.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/util/MathUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
7 files changed, 614 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/22235/9
-- 
To view, visit http://gerrit.cloudera.org:8080/22235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11228/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 19:47:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 4:

Fixed typo in commit message.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 20:44:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 1:

(1 comment)

gerrit-auto-critic failed. You can reproduce it locally using command:

  python3 bin/jenkins/critique-gerrit-review.py --dryrun

To run it, you might need a virtual env with Python3's venv installed.

http://gerrit.cloudera.org:8080/#/c/22235/1/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/1/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@451
PS1, Line 451:       long numGroupsForUniqueTuples = estimateNumGroups(exprsWithUniqueTupleId, aggInputCardinality);
line too long (101 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 19:32:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 01:41:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 07:36:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 7: Code-Review+2

(2 comments)

Thank you Jason and Csaba! Carry the +2 vote.

http://gerrit.cloudera.org:8080/#/c/22235/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/22235/6//COMMIT_MSG@18
PS6, Line 18: generalizes and replac
> nit: generalizes/replaces
Done


http://gerrit.cloudera.org:8080/#/c/22235/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@482
PS6, Line 482:     // and aggInputCardinality is unknown.
> nit: missing "is"
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 19:19:34 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 9: Code-Review+2

(1 comment)

Done.

http://gerrit.cloudera.org:8080/#/c/22235/8/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/8/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@301
PS8, Line 301: 
             :       if (LOG.isTraceEnabled()) {
             :         LOG.trace("{} aggPhase={
> Oops, this precondition is wrong. In case of non-grouping aggregation, numG
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 01:40:24 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17727/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 20:05:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17748/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 19:44:58 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11230/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 07:24:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Jason Fehr, Pranav Lodha, Michael Smith, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestValidCardinality() and
MathUtil.saturatingMultiplyCardinalities(). Both function validates that
the function arguments are valid cardinality number.
smallestValidCardinality() correctly compares two cardinality numbers
and return the smallest and valid one. It generalize and replace static
function PlanNode.capCardinalityAtLimit().
saturatingMultiplyCardinalities() adds validation and normalization over
MathUtil.saturatingMultiply().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added more preconditions in AgggregationNode.java.
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/util/MathUtil.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/util/MathUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
7 files changed, 617 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/22235/5
-- 
To view, visit http://gerrit.cloudera.org:8080/22235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 6:

Csaba give a feedback that we should not normalize accepting value < -1 for any cardinality operation.
Patch set 4 add Precondition into smallestValidCardinality().


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 20 Dec 2024 18:46:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Jason Fehr, Pranav Lodha, Michael Smith, Csaba Ringhofer, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestValidCardinality() and
MathUtil.saturatingMultiplyCardinalities(). Both function validates that
the function arguments are valid cardinality number.
smallestValidCardinality() correctly compares two cardinality numbers
and return the smallest and valid one. It generalize and replace static
function PlanNode.capCardinalityAtLimit().
saturatingMultiplyCardinalities() adds validation and normalization over
MathUtil.saturatingMultiply().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added more preconditions in AgggregationNode.java.
- Added CardinalityTest.testSmallestValidCardinality and
  MathUtilTest.testSaturatingMultiplyCardinality.
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/util/MathUtil.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M fe/src/test/java/org/apache/impala/util/MathUtilTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
7 files changed, 617 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/22235/6
-- 
To view, visit http://gerrit.cloudera.org:8080/22235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Jason Fehr, Pranav Lodha, Michael Smith, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestiValidCardinality() function that correctly compares
two cardinality numbers and return the smallest and valid one. It
generalize and replace static function PlanNode.capCardinalityAtLimit().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
5 files changed, 577 insertions(+), 43 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Pranav Lodha, Michael Smith, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing PlanNode.smallestCardinality()
function that correctly compares two cardinality numbers and return the
smallest and valid one.

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
3 files changed, 542 insertions(+), 32 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/17739/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 20:11:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/22235/8/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:

http://gerrit.cloudera.org:8080/#/c/22235/8/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@301
PS8, Line 301:       Preconditions.checkState(numGroups <= aggInputCardinality_,
             :           "numGroups (%s) is more than aggInputCardinality_ (%s)", numGroups,
             :           aggInputCardinality_);
Oops, this precondition is wrong. In case of non-grouping aggregation, numGroups = 1 and aggInputCardinality_ can be -1.
I will remove this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 01:35:37 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11230/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 21 Dec 2024 01:41:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Jason Fehr, Pranav Lodha, Michael Smith, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................

IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

An incomplete COMPUTE STATS during data loading reveal a bug in
AggregationNode.java where estimateNumGroups() can return value less
than -1.

This patch fix the bug by implementing
PlanNode.smallestValidCardinality() function that correctly compares
two cardinality numbers and return the smallest and valid one. It
generalize and replace static function PlanNode.capCardinalityAtLimit().

Reorder logic of tuple-based estimation from IMPALA-13405 such that
negative estimate is handled properly.

Testing:
- Added test in resource-requirements.test that will consistently fail
  without this fix.
- Pass testResourceRequirement.

Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
5 files changed, 577 insertions(+), 43 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Jason Fehr (Code Review)" <ge...@cloudera.org>.
Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 30 Dec 2024 19:22:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has removed a vote on this change.

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
-- 
To view, visit http://gerrit.cloudera.org:8080/22235
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-13622: Fix negative cardinality bug in AggregationNode.java

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22235 )

Change subject: IMPALA-13622: Fix negative cardinality bug in AggregationNode.java
......................................................................


Patch Set 10:

> Patch Set 10: Verified-1
> 
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11255/

It looks like COMPUTE STATS failed on tpcds.store_sales.
The telltale sign is this 'unknown' from Actual result of https://jenkins.impala.io/job/ubuntu-20.04-from-scratch/4076/testReport/junit/org.apache.impala.planner/PlannerTest/testFkPkJoinDetectionWithHDFSNumRowsEstDisabled/

|--01:SCAN HDFS [tpcds.store_returns]
|     HDFS partitions=1/1 files=1 size=31.19MB
|     predicates: sr_return_quantity < CAST(10 AS INT)
|     stored statistics:
|       table: rows=unavailable size=31.19MB
|       columns: all
|     extrapolated-rows=disabled max-scan-range-rows=unavailable
|     mem-estimate=80.00MB mem-reservation=8.00MB thread-reservation=1
|     tuple-ids=1 row-size=88B cardinality=unavailable
|     in pipelines: 01(GETNEXT)

I will try to rerun this once more. If any planner test fail again with missing stats, https://gerrit.cloudera.org/c/22231 must be prioritized.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib862a010b2094daa2cbdd5d555e46443009672ad
Gerrit-Change-Number: 22235
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Pranav Lodha <pr...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 02:03:10 +0000
Gerrit-HasComments: No