Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10888 )
Change subject: IMPALA-7260: Fix decimal binary predicates ...................................................................... Patch Set 1: (3 comments) looks good, just have a couple of questions about testing. http://gerrit.cloudera.org:8080/#/c/10888/1/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/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@265 PS1, Line 265: AnalyzesOk("select cast(1 as decimal(38,37)) " + operator + " cast(2 as bigint)"); add a comment explaining what this is trying to test. http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2418 PS1, Line 2418: testDecimalExpr(decimal_5_5 + " + cast(1 as tinyint)", these tests look more specific than the added ones since the return type is checked. coincidentally, did we just get unlucky here that the expected return types happened to be the min resolution type as well? http://gerrit.cloudera.org:8080/#/c/10888/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2496 PS1, Line 2496: public void TestDecimalFunctions() throws AnalysisException { worth it to add cases here for non-binary predicates? -- To view, visit http://gerrit.cloudera.org:8080/10888 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icc442e84cdff74a376ba25bd9897b0b4df5cf0b4 Gerrit-Change-Number: 10888 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Mon, 09 Jul 2018 15:45:53 +0000 Gerrit-HasComments: Yes
