Shant Hovsepian has posted comments on this change. ( http://gerrit.cloudera.org:8080/16349 )
Change subject: IMPALA-7310: Partial fix for NDV cardinality with NULLs. ...................................................................... Patch Set 11: (10 comments) Yeah I guess some of those comments in the tests came from different patches, all cleaned up now. http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@98 PS10, Line 98: adjustNumD > Maybe renamed as getNumDistinctValuesAdjusted(). made it adjustNumDistinctValues, since this is a private method want to avoid the get/set verbs as not to conflate with standard conventions for public methods. http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java File fe/src/main/java/org/apache/impala/catalog/ColumnStats.java: http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/main/java/org/apache/impala/catalog/ColumnStats.java@191 PS10, Line 191: > nit: seems like a move of the method in the module. Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java File fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java@211 PS10, Line 211: verifySelectCol("nullrows", "null_str", > This comment can be removed. Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java File fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java: http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java@182 PS10, Line 182: // NDV(blanks) = 1, add 1 for nulls : // Bug: See IMPALA-7310, IMPALA-8094 : //verifyNdvStmt("SELECT blanks FROM functional.nullrows", 2); > Seems like these lines can be removed. Actually no this is a different issue, just adjusted the references. http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java File fe/src/test/java/org/apache/impala/planner/CardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@132 PS10, Line 132: group_str h > This comment is not accurate. Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@136 PS10, Line 136: null_str is al > Seems like the reference to c is not right here. Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@138 PS10, Line 138: i > same here. Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@140 PS10, Line 140: (g > same Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@140 PS10, Line 140: i > same here Done http://gerrit.cloudera.org:8080/#/c/16349/10/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@182 PS10, Line 182: = 1 > Maybe as // NDV(id) = 26, ndv(null_str) = 1, NDV(id)*ndv(null_str) = 26. Done -- To view, visit http://gerrit.cloudera.org:8080/16349 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec967053b4991f8c67cde62adf003cbd3f429032 Gerrit-Change-Number: 16349 Gerrit-PatchSet: 11 Gerrit-Owner: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 31 Aug 2020 19:11:42 +0000 Gerrit-HasComments: Yes
