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 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java: http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java@69 PS10, Line 69: nor or partitions > Nit: "nor are partitions" Done http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaHdfsScanRel.java@97 PS10, Line 97: int totalCols = table.getColumns().size(); > For my own understanding: Is it true that totalCols == numFields? Yeah, I think so. This *might* be an issue when we deal with Acid tables that have virtual columns? But since we're not doing that right now and I haven't tested with that yet, it doesn't make sense to have 2 separate variables. I'm not sure I changed this as you desired. But I did get rid of the extra variable http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java: http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@70 PS10, Line 70: // O BENEVOLENT REVIEWER AND CODE INSPECTOR... : // TODO: Please hold off on reviewing this file. I held off on cleaning this up until : // this gets past the experimental stage. Some of the code in SingleNodePlanner : // is duplicated here, so this will involve a general rewrite. After more Calcite : // code gets committed and the planner works for a good portion of the queries, this : // will get rewritten into its final form. > Is this comment still true? Are there rewrites to come for this file? Sigh, unfortunately, yes, it's still true. My goal with this commit was to get a first pass Calcite commit in and make as few changes to Impala under "fe" as I possibly can. This allows the code review to be a bit simpler. The code in here is mostly common code with existing Impala code as all this happens after we have done the conversion into PlanNode. So to do this right, the code under fe/.../planner/* should be refactored. I do want to do this later, which is why I left this comment. I'm open to doing this sooner rather than later if you think though. http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java: http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@107 PS10, Line 107: case DECIMAL: : RelDataType decimalDefinedRetType = factory.createSqlType(SqlTypeName.DECIMAL, : scalarType.decimalPrecision(), scalarType.decimalScale()); : return factory.createTypeWithNullability(decimalDefinedRetType, true); : case VARCHAR: : return createCharType(factory, SqlTypeName.VARCHAR, scalarType.getLength()); : case CHAR: : return createCharType(factory, SqlTypeName.CHAR, scalarType.getLength()); > If I understand this right, we could omit DECIMAL, VARCHAR, and CHAR from t In this commit, you are correct, so I shall remove it. I think I need to put this back in future commits, but I'll do that when the time comes. http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeConverter.java@169 PS10, Line 169: Charset charSetName = Charset.forName(ConversionUtil.NATIVE_UTF16_CHARSET_NAME); > Does this charset do anything for execution? Nah, prolly not. I put this in because I saw this used in another project that used Calcite, but I'm gonna delete this unless we see a need for it in the future. http://gerrit.cloudera.org:8080/#/c/21109/10/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/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@40 PS10, Line 40: private static final int MAX_BINARY_PRECISION = Integer.MAX_VALUE; : private static final int MAX_TIMESTAMP_PRECISION = 15; : private static final int MAX_TIMESTAMP_WITH_LOCAL_TIME_ZONE_PRECISION = 15; // nanos : private static final int DEFAULT_TINYINT_PRECISION = 3; : private static final int DEFAULT_SMALLINT_PRECISION = 5; : private static final int DEFAULT_INTEGER_PRECISION = 10; : private static final int DEFAULT_BIGINT_PRECISION = 19; : private static final int DEFAULT_FLOAT_PRECISION = 7; : private static final int DEFAULT_DOUBLE_PRECISION = 15; > Could you include a comment that provides a quick definition of precision a Done http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java: http://gerrit.cloudera.org:8080/#/c/21109/10/java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java@23 PS10, Line 23: * ImpalaConformance describes supported Calcite features. > It would be nice to point to some resource that gives more detail about the Done http://gerrit.cloudera.org:8080/#/c/21109/10/tests/custom_cluster/test_calcite_planner.py File tests/custom_cluster/test_calcite_planner.py: http://gerrit.cloudera.org:8080/#/c/21109/10/tests/custom_cluster/test_calcite_planner.py@39 PS10, Line 39: def test_calcite_frontend(self, vector): > One thing we can do is add "unique_database" to this list of arguments and Done http://gerrit.cloudera.org:8080/#/c/21109/10/tests/custom_cluster/test_calcite_planner.py@40 PS10, Line 40: self.create_impala_client() > Nit: I'm pretty sure you don't need this. Done -- 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: 10 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Fri, 22 Mar 2024 19:46:44 +0000 Gerrit-HasComments: Yes
