Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21109 )
Change subject: IMPALA-12872: Use Calcite for optimization - part 1: simple queries ...................................................................... Patch Set 20: (5 comments) http://gerrit.cloudera.org:8080/#/c/21109/21/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java: http://gerrit.cloudera.org:8080/#/c/21109/21/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@99 PS21, Line 99: CalciteMetadataHandler mdHandler = > optional: other steps separate the constructor and doing the actual work - This is a good idea and I started to implement it...and then figured out why I did it this way. I wanted to keep all member variables final. But there wasn't a good way to put any of the calls into a loadTables() call and still keep all member variables final. If you think there's a better way to restructure this, let me know. http://gerrit.cloudera.org:8080/#/c/21109/21/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java: http://gerrit.cloudera.org:8080/#/c/21109/21/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalcitePhysPlanCreator.java@50 PS21, Line 50: AuthorizationFactory authzFactory = : AuthorizationUtil.authzFactoryFrom(BackendConfig.INSTANCE); > Does authorization take place in this step? Is it expected to work? Yeah, authorization will happen earlier. It's not implemented yet. This probably will be done when we grab the table names in the MetadataLoader class, so as you said, at validation time. This is just a placeholder in order to instantiate the Analyzer class, which, unfortunately, is still needed for now. This will need to be refactored. Filed IMPALA-13011 for this http://gerrit.cloudera.org:8080/#/c/21109/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java: http://gerrit.cloudera.org:8080/#/c/21109/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@35 PS20, Line 35: ImpalaTypeSystemImpl > I don't mean to solve all type system related questions in this patch, but Sigh, you caught me on something I haven't researched that much... I have much of the code working (with tpcds and tpch) and the values worked as/is. I grabbed this code originally from Hive and matched their values, figuring that was the best thing to do. Looking at the code you provided: All upcasts are going to be done external to Calcite and based on Impala rules. So it might not even be necessary to have this method for float and double But I'm pretty sure we're gonna need it though for Decimal types since Max Scale and Max Precision are gonna need to be baked into the validation step (not verified, but this makes sense to me) So now I'm in an awkward place. I tried to match Hive. I don't necessarily have the right definition. But I'd hate to leave it blank. I suppose I could throw an exception if it's called for double or float, but that doesn't seem right either. I also suppose I could just put in this explanation as I'm telling you now? And re-explore later (and file a Jira)? http://gerrit.cloudera.org:8080/#/c/21109/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@48 PS20, Line 48: private static final int DEFAULT_FLOAT_PRECISION = 7; > This is still not clear to me in the float/double case. They have fixed byt Addressed in comment above http://gerrit.cloudera.org:8080/#/c/21109/20/testdata/workloads/functional-query/queries/QueryTest/calcite.test File testdata/workloads/functional-query/queries/QueryTest/calcite.test: http://gerrit.cloudera.org:8080/#/c/21109/20/testdata/workloads/functional-query/queries/QueryTest/calcite.test@47 PS20, Line 47: decimal_tbl > This file was not updated in the last patch. Whoops, missed this (only added files under the java directory). Added now. -- To view, visit http://gerrit.cloudera.org:8080/21109 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I453fd75b7b705f4d7de1ed73c3e24cafad0b8c98 Gerrit-Change-Number: 21109 Gerrit-PatchSet: 20 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[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-Comment-Date: Wed, 17 Apr 2024 19:35:34 +0000 Gerrit-HasComments: Yes
