Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16182 )
Change subject: IMPALA-9974: Join elimination based on referential integrity. ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/16182/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16182/4//COMMIT_MSG@32 PS4, Line 32: - Q72 74.3s -> 24.8s It seems q72 and q84 don't show up in the plan changes below .. are we tracking those tests ? Also q82 plan diff doesn't show any join elimination. Perhaps the fk-pk constraints defined in the performance run were different from the constraints here. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java File fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java: http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java@53 PS4, Line 53: correlated Is the code meant to handle correlated subqueries also ? The isRewriteable() method below has a check 'if (!t.isCorrelated()' .. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java@154 PS4, Line 154: static Expr wrapWithIsNotNull(Expr slot, Expr whereExpr) { In case one or more of these columns have an IS NOT NULL predicate already in the original query, this will add an extra one. You could check whether the slot is already not-nullable before adding. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/catalog/Column.java File fe/src/main/java/org/apache/impala/catalog/Column.java: http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/catalog/Column.java@47 PS4, Line 47: protected boolean isUnique_ = false; It would be good to add a comment here or in the accessor method that the uniqueness is derived from the primary key which is an Informational constraint only (as opposed to an enforced UNIQUE constraint). http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java File fe/src/test/java/org/apache/impala/catalog/CatalogTest.java: http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@519 PS4, Line 519: //assertFalse(idCol.isNullable()); nit: remove http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@528 PS4, Line 528: //assertFalse(aCol.isNullable()); nit: remove -- To view, visit http://gerrit.cloudera.org:8080/16182 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd07d45e419175af7d62f6035ba51be841ebc074 Gerrit-Change-Number: 16182 Gerrit-PatchSet: 4 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: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 19 Oct 2020 00:58:37 +0000 Gerrit-HasComments: Yes
