uros-b commented on code in PR #59463:
URL: https://github.com/apache/spark/pull/59463#discussion_r4283590544
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2228,7 +2228,13 @@ class AstBuilder extends DataTypeAstBuilder
if (ctx.groupingExpressionsWithGroupingAnalytics.isEmpty) {
val groupByExpressions: Seq[Expression] =
ctx.groupingExpressions.asScala.map { n: NamedExpressionContext =>
- if (!allowNamedGroupingExpressions && (n.name != null ||
n.identifierList != null)) {
+ lazy val groupingExpression = visitNamedExpression(n)
Review Comment:
The rejection ignores `spark.sql.groupByOrdinal`, so with ordinals turned
off it rejects an alias that is not ambiguous at all. Please see notes below:
- `replaceIntegerLiteralWithOrdinal` turns a bare `2` into an ordinal only
`if conf.groupByOrdinal`. The new guard matches `Alias(Literal(_: Int,
IntegerType), _)` with no such condition.
- With `spark.sql.groupByOrdinal=false`, `TABLE t |> AGGREGATE sum(x) AS s
GROUP BY 2` groups by the constant `2`. Before this PR, `... GROUP BY 2 AS grp`
did the same, under the name `grp`. Neither form is an ordinal, yet the second
now fails with `PARSE_SYNTAX_ERROR`.
- The conf doc justifies the rejection "because a bare integer literal there
is an ordinal". That is false in this configuration.
- Ask:
- Add `if conf.groupByOrdinal` to the guard's `Alias(Literal(...))` case,
with a `withSQLConf(SQLConf.GROUP_BY_ORDINAL.key -> "false")` test.
- If the unconditional rule is intended, reword the conf doc and the
migration note so they no longer tie the rule to ordinal semantics.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2228,7 +2228,13 @@ class AstBuilder extends DataTypeAstBuilder
if (ctx.groupingExpressionsWithGroupingAnalytics.isEmpty) {
val groupByExpressions: Seq[Expression] =
ctx.groupingExpressions.asScala.map { n: NamedExpressionContext =>
- if (!allowNamedGroupingExpressions && (n.name != null ||
n.identifierList != null)) {
+ lazy val groupingExpression = visitNamedExpression(n)
+ val allowAlias = allowNamedGroupingExpressions &&
(groupingExpression match {
+ case Alias(Literal(_: Int, IntegerType), _) =>
+
conf.getConf(SQLConf.LEGACY_ALLOW_ALIASED_PIPE_AGGREGATE_ORDINALS)
+ case _ => true
+ })
+ if (!allowAlias && (n.name != null || n.identifierList != null)) {
Review Comment:
In pipe `AGGREGATE` the error calls the alias "extra input", but aliases on
grouping expressions are valid, documented pipe syntax. Please see notes below:
- `TABLE t |> AGGREGATE sum(x) AS s GROUP BY 2 AS grp` reports `Syntax error
at or near 'grp': extra input 'grp'`, with no position (`start = Origin()`,
empty `queryContext`). In the same clause, `GROUP BY 1 + 1 AS grp` parses fine.
- docs/sql-pipe-syntax.md documents `GROUP BY <grouping_expr> [AS alias]`
and says "Aliases can be assigned directly on grouping expressions". It
separately says `GROUP BY` may use ordinals. Nothing tells a user that the two
cannot be combined.
- Reusing regular `GROUP BY`'s text matches only on the surface. Regular
`GROUP BY` rejects every alias, so "extra input" is accurate there.
- Inside `visitOperatorPipeAggregate`, every other pipe-`AGGREGATE` `GROUP
BY` restriction raises
`UNSUPPORTED_FEATURE.PIPE_OPERATOR_AGGREGATE_UNSUPPORTED_CASE` (0A000) through
`pipeOperatorAggregateUnsupportedCaseError(case, ctx)`, which also carries the
clause position. Those restrictions are ALL, CUBE, ROLLUP, GROUPING SETS,
GROUPING/GROUPING_ID and window functions.
- Ask:
- Raise a pipe-specific error that names the restriction, for example an
alias on a `GROUP BY` ordinal.
- At a minimum, add a sentence to the `AGGREGATE` section of
docs/sql-pipe-syntax.md saying an ordinal cannot carry an alias.
- If the error changes, update the migration note and the PR description,
which both name `PARSE_SYNTAX_ERROR`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
