impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-1767 Adds predicate to test boolean values true, false, unknown.
Date Fri, 22 Sep 2017 06:20:16 GMT
Vuk Ercegovac has posted comments on this change. ( )

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

Patch Set 4:

File fe/src/main/cup/sql-parser.cup:
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.
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.
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()
done. changed to consistently named method. we can change them all in one go to something
else if needed.
PS1, Line 617:     AnalysisError("select 1 from functional.allcomplextypes where int_map_col
is null",
> use Java camel-case style: rhsOptions, lhsOptions
PS1, Line 628:   public void TestBoolValueExpression() throws AnalysisException {
> rhsTruthVals? these are definitely not literals
PS1, Line 629:     String[] rhsOptions = new String[] {"true", "false", "unknown"};
> camel case
PS1, Line 648:       for (String lhs : lhsOptions) {
> also test the NOT variants since those produce a FunctionCallExpr with a di
File fe/src/test/java/org/apache/impala/analysis/
PS1, Line 1449:     ArrayList<String> boolTestVals = new ArrayList<String>();
> Rename literals to boolTestOperations or truthTestVals? Most of these are a
done. went with boolTestVals
File fe/src/test/java/org/apache/impala/analysis/
PS1, Line 1257:     // Boolean value expression (expanded to istrue/false).
> Truth value test
changed test to expression to be consistent.
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
done. moved the expression tests to and expanded them a bit.

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: 4
Gerrit-Owner: Vuk Ercegovac <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Vuk Ercegovac <>
Gerrit-Comment-Date: Fri, 22 Sep 2017 06:20:16 +0000
Gerrit-HasComments: Yes

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