----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72423/#review220479 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 3593 (patched) <https://reviews.apache.org/r/72423/#comment308908> Can we add a comment to this method ('returns null if there are not constraints')? ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java Lines 3594 (patched) <https://reviews.apache.org/r/72423/#comment308909> nit. Indentation seems off. ql/src/java/org/apache/hadoop/hive/ql/parse/type/ConstraintExprGenerator.java Lines 40 (patched) <https://reviews.apache.org/r/72423/#comment308907> Please, add a short comment to this class. ql/src/java/org/apache/hadoop/hive/ql/parse/type/ConstraintExprGenerator.java Lines 55 (patched) <https://reviews.apache.org/r/72423/#comment308905> nit. unnecessary newline ql/src/java/org/apache/hadoop/hive/ql/parse/type/ConstraintExprGenerator.java Lines 104 (patched) <https://reviews.apache.org/r/72423/#comment308906> Instead of creating a new method `createColumnRefExpr` in the `TypeCheckProcFactory` (we need to simplify that factory rather than continue adding to it), let's just call `typeCheckProcFactory.exprFactory.toExpr()` since we have access (and we have already parsed the columns). ql/src/java/org/apache/hadoop/hive/ql/parse/type/ExprNodeTypeCheck.java Lines 80 (patched) <https://reviews.apache.org/r/72423/#comment308903> Instead of returning the generator, we should just return a ExprNodeDesc (to make it consistent with other methods in this class). Additionally, that way we do not need to expose the generator externally and the utility method would be more self contained. ql/src/java/org/apache/hadoop/hive/ql/parse/type/RexNodeTypeCheck.java Lines 71 (patched) <https://reviews.apache.org/r/72423/#comment308902> Instead of returning the generator, we should just return a RexNode (to make it consistent with other methods in this class). ql/src/java/org/apache/hadoop/hive/ql/parse/type/TypeCheckProcFactory.java Lines 1458 (patched) <https://reviews.apache.org/r/72423/#comment308904> - Jesús Camacho Rodríguez On April 23, 2020, 1:47 p.m., Krisztian Kasa wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72423/ > ----------------------------------------------------------- > > (Updated April 23, 2020, 1:47 p.m.) > > > Review request for hive, Jesús Camacho Rodríguez and Vineet Garg. > > > Bugs: HIVE-23089 > https://issues.apache.org/jira/browse/HIVE-23089 > > > Repository: hive-git > > > Description > ------- > > Add constraint checks to CBO plan > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 7b2e201e5a > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java > 0de3730351 > > ql/src/java/org/apache/hadoop/hive/ql/parse/type/ConstraintExprGenerator.java > PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/parse/type/ExprNodeTypeCheck.java > 3e3d331412 > ql/src/java/org/apache/hadoop/hive/ql/parse/type/RexNodeTypeCheck.java > c9131ec018 > ql/src/java/org/apache/hadoop/hive/ql/parse/type/TypeCheckProcFactory.java > e16966e999 > ql/src/test/results/clientnegative/update_notnull_constraint.q.out > 32905378e7 > ql/src/test/results/clientpositive/llap/check_constraint.q.out bc5d361859 > > > Diff: https://reviews.apache.org/r/72423/diff/1/ > > > Testing > ------- > > mvn test -Dtest.output.overwrite -DskipSparkTests > -Dtest=TestMiniLlapLocalCliDriver -Dqfile=check_constraint.q,sort_acid.q -pl > itests/qtest -Pitests > mvn test -Dtest.output.overwrite -DskipSparkTests > -Dtest=TestNegativeCliDriver -Dqfile=update_notnull_constraint.q -pl > itests/qtest -Pitests > > > Thanks, > > Krisztian Kasa > >
