Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21575 )
Change subject: IMPALA-13221: Calcite: Enable tpcds and tpch queries ...................................................................... Patch Set 17: (6 comments) I'm struggling on this review. Smaller reviews can handle multiple unrelated things. The bigger a code change gets, the more coherent it needs to be. This one is too big for the number of distinct things it is trying to do. I think splitting off some coherent chunks would leave a smaller grab-bag of random stuff that gets small enough to work. Here's what I would split off: 1. Parser change 2. Datetime intervals 3. Avoid collisions in table aliases Unless the FeFsTable thing does something useful, maybe split that off. The remaining thing would be a grab bag of planner rules to enable TPC-DS queries (e.g. CNF, minus to distinct, various new functions / operators, etc). I think I can live with that. http://gerrit.cloudera.org:8080/#/c/21575/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21575/17//COMMIT_MSG@49 PS17, Line 49: - Change HdfsTable to more generic FeFsTable This allows us to handle Iceberg tables, but we immediately disable IcebergTable with IMPALA-13425. Does this do something? http://gerrit.cloudera.org:8080/#/c/21575/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java: http://gerrit.cloudera.org:8080/#/c/21575/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/FunctionResolver.java@140 PS17, Line 140: // TODO: file Jira IMPALA-13435 http://gerrit.cloudera.org:8080/#/c/21575/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/ImpalaBaseTableRef.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/ImpalaBaseTableRef.java: http://gerrit.cloudera.org:8080/#/c/21575/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/ImpalaBaseTableRef.java@33 PS12, Line 33: public ImpalaBaseTableRef(TableRef tableRef, Path resolvedPath, : SimplifiedAnalyzer basicAnalyzer) throws ImpalaException { : super(tableRef, resolvedPath); : // Impala's table uniqueAlias is within the scope of each Analyzer. : // Since Impala uses a separate Analyzer instance for each query block : // it can maintain the uniqueness. However, since the Calcite planner : // uses a single SimplifiedAnalyzer for entire query and there are no : // longer separate query blocks (they have already been unnested), it needs : // to make the alias globally unique. : Preconditions.checkState(aliases_.length > 0); : aliases_[0] = basicAnalyzer.getUniqueTableAlias(getUniqueAlias()); : } > Sigh, maybe I don't understand Calcite well enough, but this might be hard Ok, I don't have a strong view about whether we can use the aliases for uniqueness, but we do need to use them in the text plan and result column names, etc. We don't need to handle that in this change. http://gerrit.cloudera.org:8080/#/c/21575/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaMinusToDistinctRule.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaMinusToDistinctRule.java: http://gerrit.cloudera.org:8080/#/c/21575/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaMinusToDistinctRule.java@47 PS17, Line 47: * https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/... : * /rel/rules/MinusToDistinctRule.java It is good to pin this to a specific version of Calcite. For example, we could change the "main" in this URL to "calcite-1.36.0". http://gerrit.cloudera.org:8080/#/c/21575/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java: http://gerrit.cloudera.org:8080/#/c/21575/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java@83 PS17, Line 83: .withCreateValuesRel(false)); If we put the IMPALA-13430 change in first, then we wouldn't need this, right? http://gerrit.cloudera.org:8080/#/c/21575/17/tests/custom_cluster/test_calcite_tpcds.py File tests/custom_cluster/test_calcite_tpcds.py: http://gerrit.cloudera.org:8080/#/c/21575/17/tests/custom_cluster/test_calcite_tpcds.py@37 PS17, Line 37: class TestTpcdsDecimalV2Query(CustomClusterTestSuite): How much time does this add to the precommit tests? (Same question for TPC-H) -- To view, visit http://gerrit.cloudera.org:8080/21575 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3107d336ac07ecd89530b640165798ec6a574f41 Gerrit-Change-Number: 21575 Gerrit-PatchSet: 17 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Wed, 16 Oct 2024 02:17:43 +0000 Gerrit-HasComments: Yes
