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: (9 comments) http://gerrit.cloudera.org:8080/#/c/16182/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/16182/4/common/thrift/ImpalaInternalService.thrift@461 PS4, Line 461: 115: optional bool disable_informational_constraints = false; By calling it 'informational_constraints' as opposed to something specific like 'referential_integrity_constraints' or foreign_key_constraints', do you think in the future we may use the same query option for other types of constraints such as NOT NULL, UNIQUE or any type of CHECK constraints ? http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2117 PS4, Line 2117: /* Is this commenting out intentional ? The trace message is useful to track predicate inferencing. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@51 PS4, Line 51: public boolean changed() { return changed_; } nit: insert a separation line between class variables and class methods. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@98 PS4, Line 98: changed_ = changed_ || !stmt.toSql(REWRITTEN).equalsIgnoreCase(stmt.toSql()); Is the string comparison the only way to detect the change status ? This could be a giant sql string. If for instance we eliminate a join or we add an IS NOT NULL predicate, couldn't we just remember that as a state transition for each subquery block ? 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@132 PS4, Line 132: .compareToIgnoreCase(pkSlot.getColumn().getName()) == 0 nit: any particular reason to use compareToIgnoreCase instead of just equalsIgnoreCase ? http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java@176 PS4, Line 176: .map(p -> p.rhsSlot()) Is the pk always expected to be on the right side due to a prior canonicalization of the conjunct ? Would be good to clarify this in a comment for this method. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java@235 PS4, Line 235: timeline_.markEvent("Computed value transfer graph for Table Constraints"); nit: This says 'Computed' ..should this be 'Starting computation of...' or something similar. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java@296 PS4, Line 296: tableRef.setJoinOp(JoinOperator.INNER_JOIN); Shouldn't this changing of join operator be done _after_ all the qualifying checks such as the 'Check for foreign keys' below.. http://gerrit.cloudera.org:8080/#/c/16182/4/fe/src/main/java/org/apache/impala/analysis/TableConstraintRewriter.java@390 PS4, Line 390: if (stmt.hasWhereClause()) { The sequence of the substitution (after table drop) seems a bit non-intuitive. The. WHERE clause substitution should preferably be before the GROUP BY and SORT, although in this case it probably doesn't matter. Also, the Analytic functions substitution is missing here. -- 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: Fri, 16 Oct 2020 22:13:02 +0000 Gerrit-HasComments: Yes
