You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Jesús Camacho Rodríguez <jc...@hortonworks.com> on 2016/11/01 00:07:41 UTC
Re: Review Request 53328: Support for standard ROLLUP syntax
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53328/#review154349
-----------------------------------------------------------
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g (line 60)
<https://reviews.apache.org/r/53328/#comment223868>
Since sets is not used in that syntax, maybe it is easier to create a parser rule that rewrites
GROUP BY (e1, e2, e3) WITH ROLLUP into ROLLUP(e1, e2, e3)
and
GROUP BY (e1, e2, e3) WITH CUBE into CUBE(e1, e2, e3)
Then the rule with the old syntax will kick in.
The advantage with this approach is that we will keep a single rule that actually generates the syntax that SemanticAnalyzer receives.
What do you think?
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g (line 64)
<https://reviews.apache.org/r/53328/#comment223865>
Instead of leaving "WITH CUBE", could you add a new "cube" syntax (similar to rollup) to support
GROUP BY CUBE(e1, e2, e3) ?
ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g (line 65)
<https://reviews.apache.org/r/53328/#comment223867>
We can remove the sets option completely from the new syntax, since we will never reach it.
- Jes�s Camacho Rodr�guez
On Oct. 31, 2016, 11:27 p.m., Vineet Garg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53328/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 11:27 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Standard ROLLUP syntax is GROUP BY ROLLUP (expression list)... but HIVE allows GROUP BY <expression list> WITH ROLLUP syntax. We would like HIVE to support standard ROLLUP syntax to allow out of the box support for TPCDS queries i.e. without rewritting them.
>
> This patach includes update to grammar to allow ROLLUP in following syntax:
>
> SELECT.....GROUP BY ROLLUP ( expr1, expr2....)
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 13e2d17
> ql/src/test/queries/clientpositive/annotate_stats_groupby.q 854e401
> ql/src/test/queries/clientpositive/cbo_rp_annotate_stats_groupby.q 3159fc7
> ql/src/test/queries/clientpositive/cte_1.q 2956339
> ql/src/test/queries/clientpositive/groupby_grouping_id1.q de4a7c3
> ql/src/test/queries/clientpositive/groupby_grouping_id2.q 5c05aad
> ql/src/test/queries/clientpositive/groupby_rollup1.q 23cac80
> ql/src/test/queries/clientpositive/infer_bucket_sort_grouping_operators.q 928f6fb
> ql/src/test/queries/clientpositive/limit_pushdown2.q 637b5b0
> ql/src/test/queries/clientpositive/vector_grouping_sets.q 09ba6b6
> ql/src/test/results/clientpositive/annotate_stats_groupby.q.out f6971a0
> ql/src/test/results/clientpositive/cbo_rp_annotate_stats_groupby.q.out f5b4375
> ql/src/test/results/clientpositive/cte_1.q.out 61fd1af
> ql/src/test/results/clientpositive/groupby_grouping_id1.q.out 136edeb
> ql/src/test/results/clientpositive/groupby_rollup1.q.out 54e1a0d
> ql/src/test/results/clientpositive/infer_bucket_sort_grouping_operators.q.out ebfce60
> ql/src/test/results/clientpositive/limit_pushdown2.q.out 2f68674
> ql/src/test/results/clientpositive/llap/cte_1.q.out e309ce8
> ql/src/test/results/clientpositive/llap/groupby_grouping_id2.q.out 544a7ae
> ql/src/test/results/clientpositive/llap/vector_grouping_sets.q.out 8e55ce3
> ql/src/test/results/clientpositive/vector_grouping_sets.q.out 4207c19
>
> Diff: https://reviews.apache.org/r/53328/diff/
>
>
> Testing
> -------
>
> Updated exsting tests to use new ROLLUP syntax in addition to non-standard syntax.
>
>
> Thanks,
>
> Vineet Garg
>
>
Re: Review Request 53328: Support for standard ROLLUP syntax
Posted by Vineet Garg <vg...@hortonworks.com>.
> On Nov. 1, 2016, 12:07 a.m., Jes�s Camacho Rodr�guez wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g, line 60
> > <https://reviews.apache.org/r/53328/diff/1/?file=1550355#file1550355line60>
> >
> > Since sets is not used in that syntax, maybe it is easier to create a parser rule that rewrites
> > GROUP BY (e1, e2, e3) WITH ROLLUP into ROLLUP(e1, e2, e3)
> > and
> > GROUP BY (e1, e2, e3) WITH CUBE into CUBE(e1, e2, e3)
> >
> > Then the rule with the old syntax will kick in.
> >
> > The advantage with this approach is that we will keep a single rule that actually generates the syntax that SemanticAnalyzer receives.
> >
> > What do you think?
I agree this would be a better approach but I am unable to figure out how to write new rule in such a way.
- Vineet
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53328/#review154349
-----------------------------------------------------------
On Oct. 31, 2016, 11:27 p.m., Vineet Garg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53328/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 11:27 p.m.)
>
>
> Review request for hive and Jes�s Camacho Rodr�guez.
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Standard ROLLUP syntax is GROUP BY ROLLUP (expression list)... but HIVE allows GROUP BY <expression list> WITH ROLLUP syntax. We would like HIVE to support standard ROLLUP syntax to allow out of the box support for TPCDS queries i.e. without rewritting them.
>
> This patach includes update to grammar to allow ROLLUP in following syntax:
>
> SELECT.....GROUP BY ROLLUP ( expr1, expr2....)
>
>
> Diffs
> -----
>
> ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 13e2d17
> ql/src/test/queries/clientpositive/annotate_stats_groupby.q 854e401
> ql/src/test/queries/clientpositive/cbo_rp_annotate_stats_groupby.q 3159fc7
> ql/src/test/queries/clientpositive/cte_1.q 2956339
> ql/src/test/queries/clientpositive/groupby_grouping_id1.q de4a7c3
> ql/src/test/queries/clientpositive/groupby_grouping_id2.q 5c05aad
> ql/src/test/queries/clientpositive/groupby_rollup1.q 23cac80
> ql/src/test/queries/clientpositive/infer_bucket_sort_grouping_operators.q 928f6fb
> ql/src/test/queries/clientpositive/limit_pushdown2.q 637b5b0
> ql/src/test/queries/clientpositive/vector_grouping_sets.q 09ba6b6
> ql/src/test/results/clientpositive/annotate_stats_groupby.q.out f6971a0
> ql/src/test/results/clientpositive/cbo_rp_annotate_stats_groupby.q.out f5b4375
> ql/src/test/results/clientpositive/cte_1.q.out 61fd1af
> ql/src/test/results/clientpositive/groupby_grouping_id1.q.out 136edeb
> ql/src/test/results/clientpositive/groupby_rollup1.q.out 54e1a0d
> ql/src/test/results/clientpositive/infer_bucket_sort_grouping_operators.q.out ebfce60
> ql/src/test/results/clientpositive/limit_pushdown2.q.out 2f68674
> ql/src/test/results/clientpositive/llap/cte_1.q.out e309ce8
> ql/src/test/results/clientpositive/llap/groupby_grouping_id2.q.out 544a7ae
> ql/src/test/results/clientpositive/llap/vector_grouping_sets.q.out 8e55ce3
> ql/src/test/results/clientpositive/vector_grouping_sets.q.out 4207c19
>
> Diff: https://reviews.apache.org/r/53328/diff/
>
>
> Testing
> -------
>
> Updated exsting tests to use new ROLLUP syntax in addition to non-standard syntax.
>
>
> Thanks,
>
> Vineet Garg
>
>