> On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > common/src/java/org/apache/hadoop/hive/common/type/HiveChar.java > > Lines 37 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103290#file2103290line37> > > > > Could we avoid adding this new constructor and let other logic in Hive > > handle this case?
I've inlined it > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java > > Lines 142 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103292#file2103292line152> > > > > You can move the project copy into the _copyNode_ method so we do not > > have (ugly) return null over there. I was considering that; but copyNode accepts only `RexNode newCondition`; but projections have an array of them... I went the other way around...since the "copyNode" is not really the needed serivce of the pointlookup rule writing - more like a benefit gained from similarity between the earlier 2 rules. I've inlined that and removed the `analyzeCondition` method; since it was already unused for the projection path... I think these things look better now. > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java > > Line 148 (original), 174 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103292#file2103292line191> > > > > Can change the numbering, i.e., 3. I've removed the numbering. I think merge IN expression is right now a no-op...so we might just remove that some day > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java > > Line 258 (original), 345 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103292#file2103292line364> > > > > Lambda? tried it; looks good at first - but the compiler keeps complaining about that it's not initialized > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java > > Lines 459 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103292#file2103292line478> > > > > This is called only once, you could just make the transformation > > in-place instead of creating new method. yes; this was left here by a refactoring step; inlined/fixed/renamed > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java > > Lines 1206 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103295#file2103295line1213> > > > > Since we transform IN into OR when we are not in CBO, do we still need > > this special handling for nulls? yes; there are some cases in which we are not opening up the IN; right now I have to add to not open it in case the left hand side is an array or a list > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java > > Lines 1372 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103295#file2103295line1379> > > > > Is there a _isConstantOrColumn1_? What is the difference? not anymore :) > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/test/results/clientpositive/join47.q.out > > Line 709 (original), 709 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103313#file2103313line709> > > > > Shouldn't we be able to close this one into an IN clause after latest > > in HivePointLookupOptimizer? its closed; the q.out was probably outdated > On Nov. 1, 2018, 10:37 p.m., Jesús Camacho Rodríguez wrote: > > ql/src/test/results/clientpositive/mapjoin47.q.out > > Line 751 (original), 751 (patched) > > <https://reviews.apache.org/r/69019/diff/4/?file=2103321#file2103321line751> > > > > same as with join47.q same here; and now it's not comparing string to ints anymore... as a second taught: ...I'm afraid that the previous expression was never returning correct results(col0 and col2 are string columns) - but the new one looks like this: ``` {(struct(UDFToDouble(_col0),UDFToDouble(_col2))) IN (const struct(100.0D,100.0D), const struct(101.0D,101.0D), const struct(102.0D,102.0D))} ``` - Zoltan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69019/#review210259 ----------------------------------------------------------- On Oct. 30, 2018, 11:32 p.m., Zoltan Haindrich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69019/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2018, 11:32 p.m.) > > > Review request for hive, Ashutosh Chauhan, Jesús Camacho Rodríguez, and > Vineet Garg. > > > Bugs: HIVE-20617 > https://issues.apache.org/jira/browse/HIVE-20617 > > > Repository: hive-git > > > Description > ------- > > For IN expressions the types were never corrected; and pointlookupoptimizer > was probably leaving behind fields already which were uncomparable; > HIVE-20296 exposed it further by changing the minimal number from 32 to 2. > > This change generalizes the retyping of constants to also run it for the IN > operator ; and also for struct-s. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/type/HiveChar.java > 29dc06dca1a > ql/src/java/org/apache/hadoop/hive/ql/ErrorMsg.java e7d71595c7a > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HivePointLookupOptimizerRule.java > 04800cca91b > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 82e975a50de > ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckCtx.java 1b56ecd0440 > ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java > 4968d16876c > ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java > c274fd7cc9b > ql/src/test/queries/clientpositive/groupby_multi_single_reducer3.q > 94aea35c422 > ql/src/test/queries/clientpositive/in_typecheck_char.q PRE-CREATION > ql/src/test/queries/clientpositive/in_typecheck_pointlook.q PRE-CREATION > ql/src/test/queries/clientpositive/in_typecheck_varchar.q PRE-CREATION > ql/src/test/queries/clientpositive/in_typecheck_x1.q PRE-CREATION > ql/src/test/queries/clientpositive/join45X.q PRE-CREATION > ql/src/test/queries/clientpositive/pcs.q 2e7eff472b5 > ql/src/test/queries/clientpositive/vector_struct_in.q 49a9374bd13 > ql/src/test/results/clientpositive/alter_partition_coltype.q.out > f6c3c5642ea > ql/src/test/results/clientpositive/in_typecheck_char.q.out PRE-CREATION > ql/src/test/results/clientpositive/in_typecheck_pointlook.q.out > PRE-CREATION > ql/src/test/results/clientpositive/in_typecheck_varchar.q.out PRE-CREATION > ql/src/test/results/clientpositive/in_typecheck_x1.q.out PRE-CREATION > ql/src/test/results/clientpositive/infer_const_type.q.out e1d7de54225 > ql/src/test/results/clientpositive/join45.q.out 47aaf7d0abc > ql/src/test/results/clientpositive/join45X.q.out PRE-CREATION > ql/src/test/results/clientpositive/join47.q.out 4d9e937815e > ql/src/test/results/clientpositive/llap/dec_str.q.out 554031e952e > ql/src/test/results/clientpositive/llap/explainuser_1.q.out f240468558e > ql/src/test/results/clientpositive/llap/lineage3.q.out 9bec309c9c9 > ql/src/test/results/clientpositive/llap/vectorization_13.q.out 4ce654f960b > ql/src/test/results/clientpositive/llap/vectorization_6.q.out a2f730beca2 > ql/src/test/results/clientpositive/llap/vectorization_8.q.out 21ce7b8ebd0 > ql/src/test/results/clientpositive/llap/vectorization_short_regress.q.out > 7f1c6a295e1 > ql/src/test/results/clientpositive/mapjoin47.q.out 294dd69de5c > ql/src/test/results/clientpositive/parquet_vectorization_13.q.out > 0efce98b555 > ql/src/test/results/clientpositive/parquet_vectorization_6.q.out > 0bb68883640 > ql/src/test/results/clientpositive/parquet_vectorization_8.q.out > 957bd7b264c > ql/src/test/results/clientpositive/pcs.q.out 4a89fb0787e > ql/src/test/results/clientpositive/ppd_udf_col.q.out 814fb5afcfd > ql/src/test/results/clientpositive/spark/cbo_simple_select.q.out > acf91bf1787 > ql/src/test/results/clientpositive/spark/parquet_vectorization_13.q.out > 38122393439 > ql/src/test/results/clientpositive/spark/parquet_vectorization_6.q.out > 6108457aad3 > ql/src/test/results/clientpositive/spark/parquet_vectorization_8.q.out > 3352dedc584 > ql/src/test/results/clientpositive/spark/spark_explainuser_1.q.out > f5a4c9ad861 > ql/src/test/results/clientpositive/spark/subquery_scalar.q.out b3252f54158 > ql/src/test/results/clientpositive/spark/vectorization_13.q.out 34ec9c42dd3 > ql/src/test/results/clientpositive/spark/vectorization_6.q.out 5679bb8cfa4 > ql/src/test/results/clientpositive/spark/vectorization_short_regress.q.out > 231dea6de3e > ql/src/test/results/clientpositive/vector_struct_in.q.out 546ef02fa42 > ql/src/test/results/clientpositive/vectorization_13.q.out 8897f8427ff > ql/src/test/results/clientpositive/vectorization_6.q.out 8dedb63e7da > ql/src/test/results/clientpositive/vectorization_8.q.out d81df76a2f6 > > > Diff: https://reviews.apache.org/r/69019/diff/4/ > > > Testing > ------- > > > Thanks, > > Zoltan Haindrich > >
