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 21: (9 comments) 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@40 PS20, Line 40: 15 > Can you add more comments about what precision means in different cases? Fo I put in a better comment, hopefully this explains it better. The values match the values existing in HiveDataTypeSystem. The float value, as I mentioned in the comment, refers to the number of digits used, much like tinyint has 3 digits. 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: // HiveDataTypeSystemImpl in the Hive github code base. > RelDataTypeSystemImpl uses 15 for float too. What is the difference behind Mentioned in above comment http://gerrit.cloudera.org:8080/#/c/21109/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@86 PS20, Line 86: INARY: > It looks scary that for some types getDefaultPrecision calls getMaxPrecisio Makes sense. http://gerrit.cloudera.org:8080/#/c/21109/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@87 PS20, Line 87: return RelDataType.PRECISION_NOT_SPECIFIED; > Shouldn't this return -1, as the user is expected to provide the precision? Done http://gerrit.cloudera.org:8080/#/c/21109/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeSystemImpl.java@89 PS20, Line 89: return RelDataType.PRECISION_NOT_SPECIFIED; > same as for CHAR Done http://gerrit.cloudera.org:8080/#/c/21109/20/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/20/java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java@121 PS20, Line 121: @Override public boolean allowGeometry() { : return false; : } > This sounds great, but I think that it is not true yet. Even HMS doesn't ha Yeah, makes sense. I didn't look at the details under the hood as to how Calcite uses this. Looking under the hood in Calcite: This is used for the parsing language. It prolly doesn't matter too much whether this is on or off, but having it off would be better for our purposes. If we left it on, there would be some parsing error messages, but if the sql parsed correctly, it would still fail at validation time. Best that all the error messages are consistent though (at validation time), so I'm changing this flag, thanks! As for Hive: Hive doesn't have a conformance file because Hive doesn't use the Calcite parser or validator. This causes some major issues in the Hive code base and lots of extra code. 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@3 PS20, Line 3: alltypes > a smaller table like alltypestiny could be queried so that the results coul I just removed this. It's not really adding any value. http://gerrit.cloudera.org:8080/#/c/21109/20/testdata/workloads/functional-query/queries/QueryTest/calcite.test@15 PS20, Line 15: drop table if exists calcite_alltypes; > This shouldn't be needed with unique_database Done http://gerrit.cloudera.org:8080/#/c/21109/20/testdata/workloads/functional-query/queries/QueryTest/calcite.test@47 PS20, Line 47: decimal_tbl > Can you also check the other missing types? 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: 21 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: Tue, 16 Apr 2024 20:53:50 +0000 Gerrit-HasComments: Yes
