Aman Sinha 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) A few things that I missed in the first pass. 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... 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 terms HdfsTable and FeView are internal classes which users don't care about. A better option would be to just not mention which types are supported and only show what is unsupported. 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 are supported. 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 both t1.ts and t2.ts .. but isn't the intent of the original test is to match t2.ts only ? Why not keep the original regex ? 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 partition by exprs for analytic functions. Could you provide the Calcite planner explain output for this ? -- 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 04:01:17 +0000 Gerrit-HasComments: Yes
