impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (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 06:20:16 GMT
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 <vercegovac@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2017 06:20:16 +0000
Gerrit-HasComments: Yes

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