Alex Behm 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 1: (11 comments) Yup, this approach is the winner! 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: bool_test_expr ::= More precisely this is a truth_test_pred, right? The rule should also be under predicate and not under non_pred_expr http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/main/cup/sql-parser.cup@2742 PS1, Line 2742: if (!l.toUpperCase().equals("UNKNOWN")) { add something like server_ident for handling UNKNOWN as an ident 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() http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@617 PS1, Line 617: String[] rhs_options = new String[] {"true", "false", "unknown"}; use Java camel-case style: rhsOptions, lhsOptions http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@628 PS1, Line 628: String literal_lhs = rhs == "unknown" ? "null" : rhs; rhsTruthVals? these are definitely not literals http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@629 PS1, Line 629: String negated_rhs = "not " + rhs; camel case http://gerrit.cloudera.org:8080/#/c/8122/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@648 PS1, Line 648: AnalysisError("select ('foo' is true)", also test the NOT variants since those produce a FunctionCallExpr with a different fn name 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> literals = new ArrayList<String>(); Rename literals to boolTestOperations or truthTestVals? Most of these are actually keywords and definitely not literals. 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 test (expanded to istrue/false). Truth value test 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 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 -- 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: 1 Gerrit-Owner: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Comment-Date: Fri, 22 Sep 2017 00:47:29 +0000 Gerrit-HasComments: Yes
