impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
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. ( )

Change subject: IMPALA-1767 Adds predicate to test boolean values true, false, unknown.

Patch Set 1:


Yup, this approach is the winner!
File fe/src/main/cup/sql-parser.cup:
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
PS1, Line 2742:       if (!l.toUpperCase().equals("UNKNOWN")) {
add something like server_ident for handling UNKNOWN as an ident
File fe/src/test/java/org/apache/impala/analysis/
PS1, Line 612:   public void TestIsNullOrBoolPredicates() throws AnalysisException {
Let's add the new tests under a separate TestTruthTestPredicate()
PS1, Line 617:     String[] rhs_options = new String[] {"true", "false", "unknown"};
use Java camel-case style: rhsOptions, lhsOptions
PS1, Line 628:       String literal_lhs = rhs == "unknown" ? "null" : rhs;
rhsTruthVals? these are definitely not literals
PS1, Line 629:       String negated_rhs = "not " + rhs;
camel case
PS1, Line 648:     AnalysisError("select ('foo' is true)",
also test the NOT variants since those produce a FunctionCallExpr with a different fn name
File fe/src/test/java/org/apache/impala/analysis/
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.
File fe/src/test/java/org/apache/impala/analysis/
PS1, Line 1257:     // Boolean value test (expanded to istrue/false).
Truth value test
PS1, Line 1258:     testToSql("select (true is true)", "SELECT (istrue(TRUE))");
Let's exhaustively try all possibilities for full coverage
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:
PS1, Line 74: # boolean test: IS [NOT] (TRUE | FALSE | UNKNOWN)
These tests using literals only are more appropriate in

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Comment-Date: Fri, 22 Sep 2017 00:47:29 +0000
Gerrit-HasComments: Yes

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