impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Date Fri, 22 Sep 2017 00:47:29 GMT
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 <vercegovac@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2017 00:47:29 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message