----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69019/#review209778 -----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java Line 1172 (original), 1181 (patched) <https://reviews.apache.org/r/69019/#comment294365> This TODO will be good to resolve. We have type already so we can return null constant of appropriate type here, no? ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java Lines 1404 (patched) <https://reviews.apache.org/r/69019/#comment294366> Add a comment: comparison of decimal and double happens in double. ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java Lines 1292-1294 (original), 1416-1418 (patched) <https://reviews.apache.org/r/69019/#comment294367> Not sure this logic is incorrect. We shall not coerce the length of constant to be same as length of type. Comparisons for 2 char types happens on stripped values. So, if constant is of smaller length then this probably won't be a problem but is unnecessary. However, if constant is longer, looks like HiveChar will truncate it and then comparison likely will be wrong. Better is to create constant char of same length as of string. ql/src/test/queries/clientpositive/in_typecheck1.q Lines 4 (patched) <https://reviews.apache.org/r/69019/#comment294369> Can you repeat this test with s varchar(1), t varchar(10); ? ql/src/test/queries/clientpositive/in_typecheck1.q Lines 7 (patched) <https://reviews.apache.org/r/69019/#comment294368> Add: select * from ax where t = 'a '; select * from ax where t = 'a '; select * from ax where t = 'a d'; RHS constant is of length 10,11,12. I expect first and second to return 2 rows, while third to return 0 rows. When t is varchar all 3 should return 0 rows. ql/src/test/queries/clientpositive/in_typecheck1.q Lines 10 (patched) <https://reviews.apache.org/r/69019/#comment294370> When s and t are varchar this should return 1 row. ql/src/test/results/clientpositive/in_typecheck1.q.out Lines 42 (patched) <https://reviews.apache.org/r/69019/#comment294362> This doesn't look correct. For char/varchar comparisons doesn't preserve trailing spaces. e.g., create table t1 (a char(3), b varchar(4)); insert into t1 values ('a','b'); select * from t1 where b = 'b'; a b select * from t1 where a = 'a'; a b Got this one on both postgres and oracle. whereas this change would return empty results in Hive i believe ql/src/test/results/clientpositive/join45.q.out Line 717 (original), 717 (patched) <https://reviews.apache.org/r/69019/#comment294363> comparison of str and integer is done in double. See e.g, infer_const_type.q.out where there is (UDFToDouble(str) = 1234.0D However, inside IN we are now casting constant to string. Granted casting to double in this scenario is debatable but Hive had this behavior since beginning and it need to be consistent inside IN or with direct comparison. ql/src/test/results/clientpositive/llap/subquery_scalar.q.out Line 371 (original), 370 (patched) <https://reviews.apache.org/r/69019/#comment294364> Unrelated to this patch, but I think subq decorrelation logic incorrectly generated this filter. It should really have been p_name is null. - Ashutosh Chauhan On Oct. 15, 2018, 6:05 a.m., Zoltan Haindrich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69019/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2018, 6:05 a.m.) > > > Review request for hive and Ashutosh Chauhan. > > > 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 > ----- > > ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java > 4968d16876c5c9cc36ec9a3ec48c2740c2c67dcd > ql/src/test/queries/clientpositive/in_typecheck1.q PRE-CREATION > ql/src/test/queries/clientpositive/in_typecheck2.q PRE-CREATION > ql/src/test/results/clientpositive/alter_partition_coltype.q.out > 5727f0a65c6e4736f41017e4e962d932dedbd6bd > ql/src/test/results/clientpositive/cbo_rp_simple_select.q.out > 43cb5ab89fdebde8be168d7837d8e54a38f4d10b > ql/src/test/results/clientpositive/cbo_simple_select.q.out > 2073c6b802a1ae0ff4228a86f18ec366ff92ab02 > ql/src/test/results/clientpositive/in_typecheck1.q.out PRE-CREATION > ql/src/test/results/clientpositive/in_typecheck2.q.out PRE-CREATION > ql/src/test/results/clientpositive/infer_const_type.q.out > 4129bd0c715635eb83c0c0d248eb43a5779c7be9 > ql/src/test/results/clientpositive/join45.q.out > 77dbaa2cd8b5be7158545c696b30dc1068238f91 > ql/src/test/results/clientpositive/join47.q.out > 2536f7f4b6e9295d1177632b7f32f0b66974e3a4 > ql/src/test/results/clientpositive/llap/cbo_simple_select.q.out > e61300b5c853eb733d4443c047344e3fc6fe0ff3 > ql/src/test/results/clientpositive/llap/dec_str.q.out > 3b7f92d735db79f7c4c0d96debe7fff8e3c05f11 > ql/src/test/results/clientpositive/llap/explainuser_1.q.out > bc1f97dd49ccc905d5e32d5a02a62bb692d444a6 > ql/src/test/results/clientpositive/llap/lineage3.q.out > cf388161272002ad6097839ceaea2bcfbcf9b7ef > ql/src/test/results/clientpositive/llap/mapjoin_hint.q.out > 3c6270a05240097e9645e664ed30e0568052d98e > ql/src/test/results/clientpositive/llap/subquery_scalar.q.out > feadbcd874818a78e2cc30b86cdebc1e4cb6a04f > ql/src/test/results/clientpositive/llap/vectorization_13.q.out > 398cb56915f1b24b7c4dc325b60cb114d7ff2b8c > ql/src/test/results/clientpositive/llap/vectorization_6.q.out > 70542ac7bd69e46098cf8158cae347e6c896c5b2 > ql/src/test/results/clientpositive/llap/vectorization_8.q.out > 662409d4f148cc3da5c4f788ddf59c6f40ede572 > ql/src/test/results/clientpositive/llap/vectorization_short_regress.q.out > a59a586144fc9dc14ac2fa87177c189feae47402 > ql/src/test/results/clientpositive/mapjoin47.q.out > c42094d7858fa70626a5184485a13fdacd45be7c > ql/src/test/results/clientpositive/parquet_vectorization_13.q.out > e60548cb779826c0e50b5086cabe0b1408f4f182 > ql/src/test/results/clientpositive/parquet_vectorization_6.q.out > 85b075666f9cdcd15b2adbab07d665b8def863c0 > ql/src/test/results/clientpositive/parquet_vectorization_8.q.out > c089aab12554caa6ea38d1a0d90abf0c050b8ba0 > ql/src/test/results/clientpositive/ppd_udf_col.q.out > dfc2d0465d49f7c77452113d3791104a05aa42ef > ql/src/test/results/clientpositive/spark/cbo_simple_select.q.out > e61300b5c853eb733d4443c047344e3fc6fe0ff3 > ql/src/test/results/clientpositive/spark/parquet_vectorization_13.q.out > 78a2428cfbd508c768100896955b8853a9bd2a50 > ql/src/test/results/clientpositive/spark/parquet_vectorization_6.q.out > 362d19c39b60ec2193fc73098787723042f53aab > ql/src/test/results/clientpositive/spark/parquet_vectorization_8.q.out > b10b550009d383f84f19ac72207a9c94b12c497d > ql/src/test/results/clientpositive/spark/subquery_scalar.q.out > af325200ba7ae7aa5ab0aa42ecf714eaa4afb122 > ql/src/test/results/clientpositive/spark/vectorization_13.q.out > a49738ecaac6324d529954ca14b952b4898e33dd > ql/src/test/results/clientpositive/spark/vectorization_6.q.out > 8f7d3f2530da2afc10e8dfb23339568279d8c454 > ql/src/test/results/clientpositive/spark/vectorization_short_regress.q.out > 3844c79e1a3e1d37b8b93e4f39bc9b52992888f3 > ql/src/test/results/clientpositive/vectorization_13.q.out > d2f34481371122b2d6a33578a68310b739eb023f > ql/src/test/results/clientpositive/vectorization_6.q.out > 99d917fdfa101cba42a1f91b5ac39d9401584a7d > ql/src/test/results/clientpositive/vectorization_8.q.out > b9b0c8af17b13cebb2301012fadf3395786b4f62 > > > Diff: https://reviews.apache.org/r/69019/diff/1/ > > > Testing > ------- > > > Thanks, > > Zoltan Haindrich > >
