Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10882 )
Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2286 PS3, Line 2286: Collections.sort(sortedExprs, new Comparator<Expr>() { > It looks like we are sorting here in order to put the decimals at the end o The code actually puts all the decimals at the beginning. If we handle non-decimals first, we will end up with a floating point type for the compatible type because of https://github.com/apache/impala/blob/fd0ba0fd2c7631d48b4cce56e60f0b9f902cc446/fe/src/main/java/org/apache/impala/catalog/ScalarType.java#L479-L480. For example: (double, double, decimal(38, 1), decimal(38, 37)) -- wrong double vs double --> compatible type: double double vs decimal(38, 1) --> compatible type: double double vs decimal(38, 7) --> compatible type: double If we group all the decimals together first, e.g. (decimal(38, 1), decimal(38, 37), double, double) -- correct decimal(38, 1) vs decimal(38, 7) --> error because of strict decimal The way the existing code was written, it can get pretty messy to do it in 2 passes especially since now we need to worry about which element to consider for the lastCompatibleExpr in the 1st pass vs the 2nd pass. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: http://gerrit.cloudera.org:8080/#/c/10882/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2451 PS3, Line 2451: 2 as decimal(38, 37) > One should be decimal(38,37) and the other one decimal(38,1) - to make sure I have test cases for that in L2464 and L2491. -- To view, visit http://gerrit.cloudera.org:8080/10882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d Gerrit-Change-Number: 10882 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Comment-Date: Sat, 07 Jul 2018 00:17:57 +0000 Gerrit-HasComments: Yes
