> On Jan. 5, 2018, 4:32 a.m., Jesús Camacho Rodríguez wrote: > > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFGrouping.java > > Line 63 (original), 63 (patched) > > <https://reviews.apache.org/r/64900/diff/2/?file=1932081#file1932081line63> > > > > I think it should be sufficient to check for LONG. > > Prasanth_J wrote: > The reason I am checking both INT and LONG here is there are many places > in compiler that assumes Int for grouping. I fixed in many places, however > there are places where string constansts "0" are initialized for > "grouping__id" column, some cases where type inferences returns int, some > cases constant folding returns int which became too intrusive of a change. So > I handled it here in the UDF. Anyway, we use getLong() to retrieve the value. > So I guess it is not a problem? As long as the bitmask is returned as long I > think it shouldn't be a problem. I will wait for another test run to confirm > this statement :) > > Jesús Camacho Rodríguez wrote: > If only LONG is used, where do you observe the issue / for which tests? I > saw that you changed it already in many places, including Calcite, thus it > should be fine? If you let me know the test, I can help taking a look too. I > agree that it might not be a problem, but I am not sure in which cases we > would end up referencing an INT, hence I just want to make sure that we are > not masking another issue. > > Prasanth_J wrote: > These are the test where I observed failures when I changed this case to > LONG only > > groupby_grouping_sets_grouping.q,groupby_grouping_window.q,groupby_rollup_empty.q,udf_width_bucket.q,vector_grouping_sets.q > > Some pointers > > https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java#L1188 > > https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java#L866 > > https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java#L3131 > > The rabbit hole went a little deep, so I handled it in the UDF. > > Jesús Camacho Rodríguez wrote: > Thanks for the links Prasanth. > For the first two, we should not need to change anything as the constant > is just a placeholder. For the third, the change should not be in that line, > but rather in: > > https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java#L3113 > (maybe a typo in the line number? :) ) > Once you upload the new patch with the fix for grouping func, I will try > to repro. > > Prasanth_J wrote: > I uploaded new patch. groupby_grouping_window.q is still failing.
Thanks Prasanth, I have been checking and this only happens when we enter the first branch in the if clause in https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java#L3113 , as apparently even with change it is interpreted as INT. Since that is the only case, I think it is OK to bring back the change to allow INT and LONG for the first parameter in the function and push the patch if tests pass. Sorry about the hassle, I just wanted to understand what was going on, as it is easy to create errors inadvertently in that part of the code. - Jesús ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64900/#review194829 ----------------------------------------------------------- On Jan. 5, 2018, 6:55 a.m., Prasanth_J wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64900/ > ----------------------------------------------------------- > > (Updated Jan. 5, 2018, 6:55 a.m.) > > > Review request for hive and Jesús Camacho Rodríguez. > > > Bugs: HIVE-18359 > https://issues.apache.org/jira/browse/HIVE-18359 > > > Repository: hive-git > > > Description > ------- > > HIVE-18359: Extend grouping set limits from int to long > > > Diffs > ----- > > ql/src/java/org/apache/hadoop/hive/ql/exec/GroupByOperator.java 8b94d1d > > ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorGroupByOperator.java > 90145e5 > ql/src/java/org/apache/hadoop/hive/ql/metadata/VirtualColumn.java 0032305 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/reloperators/HiveGroupingID.java > adcda26 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/rules/HiveExpandDistinctAggregatesRule.java > 89c5c23 > > ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/translator/HiveGBOpConvUtil.java > 6f4188c > ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java 28b4cfe > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 5a88a96 > ql/src/java/org/apache/hadoop/hive/ql/plan/GroupByDesc.java 9d4ad22 > ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFGrouping.java > cee0e14 > ql/src/test/queries/clientpositive/cte_1.q 15d3f06 > ql/src/test/queries/clientpositive/groupingset_high_columns.q PRE-CREATION > ql/src/test/results/clientpositive/annotate_stats_groupby.q.out ed3d594 > ql/src/test/results/clientpositive/annotate_stats_groupby2.q.out ffcb20f > ql/src/test/results/clientpositive/auto_join25.q.out 063d3ca > ql/src/test/results/clientpositive/cte_1.q.out 9374a32 > ql/src/test/results/clientpositive/groupby_cube1.q.out e5ece81 > ql/src/test/results/clientpositive/groupby_cube_multi_gby.q.out 9a6457c > ql/src/test/results/clientpositive/groupby_grouping_id3.q.out f13b6e5 > ql/src/test/results/clientpositive/groupby_grouping_sets1.q.out d70f065 > ql/src/test/results/clientpositive/groupby_grouping_sets2.q.out 453b9f7 > ql/src/test/results/clientpositive/groupby_grouping_sets3.q.out be8d20e > ql/src/test/results/clientpositive/groupby_grouping_sets4.q.out 0c6ead9 > ql/src/test/results/clientpositive/groupby_grouping_sets5.q.out 0bb12e1 > ql/src/test/results/clientpositive/groupby_grouping_sets6.q.out 5b990a1 > ql/src/test/results/clientpositive/groupby_grouping_sets_grouping.q.out > 1f2f86b > ql/src/test/results/clientpositive/groupby_grouping_sets_limit.q.out > b25b0e5 > ql/src/test/results/clientpositive/groupby_grouping_window.q.out 32135e4 > ql/src/test/results/clientpositive/groupby_rollup1.q.out bc1d8a9 > ql/src/test/results/clientpositive/groupby_rollup_empty.q.out 5db3184 > ql/src/test/results/clientpositive/groupingset_high_columns.q.out > PRE-CREATION > > ql/src/test/results/clientpositive/infer_bucket_sort_grouping_operators.q.out > 5f1d264 > ql/src/test/results/clientpositive/limit_pushdown2.q.out cdd221b > ql/src/test/results/clientpositive/llap/cte_1.q.out ddef9db > ql/src/test/results/clientpositive/llap/groupby_rollup_empty.q.out 061b0d7 > > ql/src/test/results/clientpositive/llap/insert_values_orig_table_use_metadata.q.out > d135f08 > ql/src/test/results/clientpositive/llap/llap_acid.q.out 38889b9 > ql/src/test/results/clientpositive/llap/llap_acid_fast.q.out 4a7297d > ql/src/test/results/clientpositive/llap/multi_count_distinct_null.q.out > 39feaec > ql/src/test/results/clientpositive/llap/sysdb.q.out 3bd407b > ql/src/test/results/clientpositive/llap/vector_groupby_cube1.q.out 39de888 > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_id1.q.out > 7224d59 > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_id2.q.out > e6075c7 > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_id3.q.out > da4b81f > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets1.q.out > d2b738b > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets2.q.out > 1877bba > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets3.q.out > 7c9f668 > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets4.q.out > 6a5e679 > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets5.q.out > 4d8fa16 > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets6.q.out > 5e9e204 > > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets_grouping.q.out > b81a0d3 > > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_sets_limit.q.out > e8ca06e > > ql/src/test/results/clientpositive/llap/vector_groupby_grouping_window.q.out > 4de6ebb > ql/src/test/results/clientpositive/llap/vector_groupby_rollup1.q.out > d1263cd > ql/src/test/results/clientpositive/perf/spark/query18.q.out 88d289c > ql/src/test/results/clientpositive/perf/spark/query22.q.out 15fe441 > ql/src/test/results/clientpositive/perf/spark/query27.q.out 627821f > ql/src/test/results/clientpositive/perf/spark/query36.q.out d313337 > ql/src/test/results/clientpositive/perf/spark/query5.q.out 14e0bdb > ql/src/test/results/clientpositive/perf/spark/query67.q.out 25d37bd > ql/src/test/results/clientpositive/perf/spark/query70.q.out 862bdb0 > ql/src/test/results/clientpositive/perf/spark/query77.q.out 6dcaf7c > ql/src/test/results/clientpositive/perf/spark/query80.q.out 606d23c > ql/src/test/results/clientpositive/perf/spark/query86.q.out 8e159fd > ql/src/test/results/clientpositive/spark/groupby_cube1.q.out fa1480e > ql/src/test/results/clientpositive/spark/groupby_rollup1.q.out 460e7db > ql/src/test/results/clientpositive/spark/limit_pushdown2.q.out f6f0043 > ql/src/test/results/clientpositive/tez/multi_count_distinct.q.out 7f7d354 > ql/src/test/results/clientpositive/vector_grouping_sets.q.out f08c270 > ql/src/test/results/clientpositive/view_cbo.q.out e10cb7a > > > Diff: https://reviews.apache.org/r/64900/diff/3/ > > > Testing > ------- > > > Thanks, > > Prasanth_J > >
