Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24528 )
Change subject: IMPALA-15135: Calcite planner: added support for Kudu tables ...................................................................... Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/24528/12/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java File fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java: http://gerrit.cloudera.org:8080/#/c/24528/12/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@85 PS12, Line 85: count(*) > nit: one more instance... Done http://gerrit.cloudera.org:8080/#/c/24528/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java: http://gerrit.cloudera.org:8080/#/c/24528/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java@74 PS12, Line 74: "HdfsTable's and FeView's." > Add Kudu table. Also, can we make these more user-friendly names ? The te The unsupported type list is still quite a bit and might also be confusing (i.e. IcebergMetadatatable). I just eliminated the last sentence because it will never have to change now and it shouldn't be too hard to deduce the problem from the error message. http://gerrit.cloudera.org:8080/#/c/24528/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java: http://gerrit.cloudera.org:8080/#/c/24528/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@123 PS12, Line 123: if (table instanceof FeView) { : throw new UnsupportedFeatureException("Views are not supported yet."); : } : > Unrelated to this patch but in CalciteTable.addTable() we are saying Views Ah, yeah, this is dead code. A view doesn't create a CalciteTable so it would never hit this code. http://gerrit.cloudera.org:8080/#/c/24528/12/testdata/workloads/functional-query/queries/QueryTest/kudu_runtime_filter_with_timestamp_conversion.test File testdata/workloads/functional-query/queries/QueryTest/kudu_runtime_filter_with_timestamp_conversion.test: http://gerrit.cloudera.org:8080/#/c/24528/12/testdata/workloads/functional-query/queries/QueryTest/kudu_runtime_filter_with_timestamp_conversion.test@30 PS12, Line 30: .*.ts > These regex changes here and other places in the patch mean it will match b One general problem I've had with the explain plans is that Calcite does not use aliases. The Calcite output that matches looks like this: RF002[min_max] -> functional_kudu.timestamp_at_dst_changes.ts I have one of two options then: I can either split off this section and have a CALCITE_PLANNER_RUNTIME_PROFILE with the full output or use regex. I'm ok with either one. Would you prefer me to do it the other way? http://gerrit.cloudera.org:8080/#/c/24528/12/testdata/workloads/functional-query/queries/QueryTest/partitioned-top-n-high-ndv.test File testdata/workloads/functional-query/queries/QueryTest/partitioned-top-n-high-ndv.test: http://gerrit.cloudera.org:8080/#/c/24528/12/testdata/workloads/functional-query/queries/QueryTest/partitioned-top-n-high-ndv.test@39 PS12, Line 39: row_regex:'| partition by: .*part1, .*part2, .*part3, .*col4, .*col5' > It wasn't clear how the changes for kudu tables in this patch affect partit This is similar to my other comment. Explain plan for this line looks like this: | partition by: sjc_db.high_ndv_cols.part1, sjc_db.high_ndv_cols.part2, sjc_db.high_ndv_cols.part3, sjc_db.high_ndv_cols.col4, sjc_db.high_ndv_cols.col5 -- To view, visit http://gerrit.cloudera.org:8080/24528 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie22a66f0edad195cade74a8447efc83a8ce526f7 Gerrit-Change-Number: 24528 Gerrit-PatchSet: 12 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Xuebin Su <[email protected]> Gerrit-Comment-Date: Sun, 12 Jul 2026 21:56:16 +0000 Gerrit-HasComments: Yes
