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/06/11 17:37:49 UTC

[Impala-ASF-CR] IMPALA-13152: Avoid NaN cost at SortNode.java

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


Change subject: IMPALA-13152: Avoid NaN cost at SortNode.java
......................................................................

IMPALA-13152: Avoid NaN cost at SortNode.java

TOP-N cost will turn into NaN if inputCardinality is equal to 0 due to
Math.log(inputCardinality). This patch fix the issue by avoiding
Math.log(0) and replace it with 0 instead.

After this patch, Instantiating BaseProcessingCost with NaN or Infinite
totalCost will throw IllegalArgumentException. In
BaseProcessingCost.getDetails(), "total-cost" is renamed to "raw-cost"
to avoid confusion with "cost-total" in ProcessingCost.getDetails().

Testing:
- Add testcase that run TOP-N query over empty table.
- Pass PlannerTest.testProcessingCost().

Change-Id: Ib49c7ae397dadcb2cb69fde1850d442d33cdf177
---
M fe/src/main/java/org/apache/impala/planner/BaseProcessingCost.java
M fe/src/main/java/org/apache/impala/planner/ProcessingCost.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds_cpu_cost/tpcds-q43-verbose.test
5 files changed, 115 insertions(+), 29 deletions(-)



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

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

[Impala-ASF-CR] IMPALA-13152: Avoid NaN cost at SortNode.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/21504 )

Change subject: IMPALA-13152: Avoid NaN cost at SortNode.java
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/16315/ : 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/21504
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib49c7ae397dadcb2cb69fde1850d442d33cdf177
Gerrit-Change-Number: 21504
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Jun 2024 18:03:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost

Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.

Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/21504 )

Change subject: IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost
......................................................................


Patch Set 3: Code-Review+1

(3 comments)

General question, don't need to be addressed in this patch. Should we distinguish between missing cardinality information and 0 cardinality? 0 cardinality would mean no data to process while missing cardinality information means we don't have cardinality information likely due to missing stats and so we could probably use some default cardinality.

http://gerrit.cloudera.org:8080/#/c/21504/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21504/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@a2231
PS3, Line 2231: 
Why is this check no longer required?


http://gerrit.cloudera.org:8080/#/c/21504/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java
File fe/src/main/java/org/apache/impala/planner/ScanNode.java:

http://gerrit.cloudera.org:8080/#/c/21504/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java@a399
PS3, Line 399: 
Why this check is no longer required?


http://gerrit.cloudera.org:8080/#/c/21504/3/fe/src/main/java/org/apache/impala/planner/ScanNode.java@a420
PS3, Line 420: 
Same question.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib49c7ae397dadcb2cb69fde1850d442d33cdf177
Gerrit-Change-Number: 21504
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jun 2024 22:38:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost

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

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21504 )

Change subject: IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib49c7ae397dadcb2cb69fde1850d442d33cdf177
Gerrit-Change-Number: 21504
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 15 Jun 2024 18:32:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost

Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.

Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21504 )

Change subject: IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost
......................................................................


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/21504/2/fe/src/main/java/org/apache/impala/planner/DataSink.java@141
PS2, Line 141:         "Processing cost of DataSink %s:%s is invalid! %s", fragment_.getId(), getLabel(),
> nit: could improve this string construction using a format string with Prec
Done


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

http://gerrit.cloudera.org:8080/#/c/21504/2/fe/src/main/java/org/apache/impala/planner/HashJoinNode.java@342
PS2, Line 342:     long outputCardinality = Math.max(0, getCardinality());
> Is there some reason we need cardinality_ to default to -1 rather than 0?
PlanNode starts with unknown cardinality (-1) until stats are analyzed. Unknown is represented as -1, and some planner code handle unknown cardinality with best guess estimate like in some ScanNode subclasses when stats are unavailable.
https://gerrit.cloudera.org/c/21504/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java#2256



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib49c7ae397dadcb2cb69fde1850d442d33cdf177
Gerrit-Change-Number: 21504
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2024 19:23:20 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost

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

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21504 )

Change subject: IMPALA-13152: Avoid NaN, infinite, and negative ProcessingCost
......................................................................


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib49c7ae397dadcb2cb69fde1850d442d33cdf177
Gerrit-Change-Number: 21504
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: David Rorke <dr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Comment-Date: Sat, 15 Jun 2024 23:36:45 +0000
Gerrit-HasComments: No