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: (2 comments) 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 > We need to think about the Calcite changes needed for introducing the alias Heh, I was thinking about this right before I read your comment. I agree this is definitely needed. It's possible that it already exists in Calcite and I've missed it, but the plugins to our code allows us to add tables to a general schema, and I haven't yet seen a way to propagate that information when the RelNodes are created. So yeah, as you mentioned, we might need Calcite community involvement here. As for your second comment, I answered that in the other comment you left with this version, so it's probably better to respond to that issue in the other comment. 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' > Could we use CALCITE_PLANNER_RESULTS section for the Calcite plan ? The ali I have had this problem in other explain plans. While the number of explain plans isn't trivial and slightly pervasive, it also was manageable to change them all. Relative note: The junit tests which test the explain plans do not check every line, so I didn't have to change anything there. For the most part, I've been doing it this way. I actually kinda felt the reverse of your last statement. Introducing a CALCITE_PLANNER_RESULTS section duplicates a lot of information. I felt that, for the most part, the explain plans are more concerned with the general Plan Node layout, runtime filters are generated, etc... Also, by having a second section, it would be easy for someone to change the original planner text here without changing the Calcite planner text. This wouldn't be found until later in testing, or even worse, with our current jenkins monitoring, after the commit is done. One last note: while it is prone to potentially passing a check on a field on the wrong table, I think an error like that would most likely be picked up causing incorrect results and failing other tests. -- 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: Mon, 13 Jul 2026 09:46:49 +0000 Gerrit-HasComments: Yes
