Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8122 )
Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown. ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2731 PS1, Line 2731: {: > More precisely this is a truth_test_pred, right? The rule should also be un Changed it to bool_value_expr to be consistent with the standard. I put this under non_pred_expr since FunctionCallExpr is an Expr, not a Predicate. The workaround I'm including here is to declare nonterminal predicate as an Expr (not a predicate)-- let me know if that's preferable. http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2742 PS1, Line 2742: RESULT = FunctionCallExpr.createExpr( > add something like server_ident for handling UNKNOWN as an ident done-- thanks for the tip, I was looking for something like that. http://gerrit.cloudera.org:8080/#/c/8122/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/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@612 PS1, Line 612: public void TestIsNullOrBoolPredicates() throws AnalysisException { > Let's add the new tests under a separate TestTruthTestPredicate() done. changed to consistently named method. we can change them all in one go to something else if needed. http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@617 PS1, Line 617: AnalysisError("select 1 from functional.allcomplextypes where int_map_col is null", > use Java camel-case style: rhsOptions, lhsOptions Done http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@628 PS1, Line 628: public void TestBoolValueExpression() throws AnalysisException { > rhsTruthVals? these are definitely not literals Done http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@629 PS1, Line 629: String[] rhsOptions = new String[] {"true", "false", "unknown"}; > camel case Done http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@648 PS1, Line 648: for (String lhs : lhsOptions) { > also test the NOT variants since those produce a FunctionCallExpr with a di Done http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@1449 PS1, Line 1449: ArrayList<String> boolTestVals = new ArrayList<String>(); > Rename literals to boolTestOperations or truthTestVals? Most of these are a done. went with boolTestVals http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1257 PS1, Line 1257: // Boolean value expression (expanded to istrue/false). > Truth value test changed test to expression to be consistent. http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1258 PS1, Line 1258: testToSql("select (true is true)", "SELECT (istrue(TRUE))"); > Let's exhaustively try all possibilities for full coverage Done http://gerrit.cloudera.org:8080/#/c/8122/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/8122/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test@74 PS1, Line 74: # boolean test: IS [NOT] (TRUE | FALSE | UNKNOWN) > These tests using literals only are more appropriate in expr-test.cc done. moved the expression tests to expr-test.cc and expanded them a bit. -- To view, visit http://gerrit.cloudera.org:8080/8122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9d5fba65ef6c87dfc55a25d2c45246f74eb48c40 Gerrit-Change-Number: 8122 Gerrit-PatchSet: 4 Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 22 Sep 2017 06:20:16 +0000 Gerrit-HasComments: Yes
