Skip to content

[SPARK-17016][SQL] Improve group-by/order-by ordinal error reporting #14594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
petermaxlee wants to merge 2 commits into from
Closed

[SPARK-17016][SQL] Improve group-by/order-by ordinal error reporting #14594

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e1b6b3
[SPARK-17016][SQL] Improve group-by/order-by ordinal error reporting
petermaxlee Aug 11, 2016
156d5d8
Delete test cases
petermaxlee Aug 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,8 +547,7 @@ class Analyzer(
case a: Aggregate if containsStar(a.aggregateExpressions) =>
if (conf.groupByOrdinal && a.groupingExpressions.exists(IntegerIndex.unapply(_).nonEmpty)) {
failAnalysis(
"Group by position: star is not allowed to use in the select list " +
"when using ordinals in group by")
"Star (*) is not allowed in select list when GROUP BY ordinal position is used")
Copy link
Contributor Author

@petermaxlee petermaxlee Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @cloud-fan / @gatorsmile do you know why star is not allowed here? I checked Postgres does allow star.

Copy link
Contributor

@cloud-fan cloud-fan Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #10731 (comment)

I think it's not about group by ordinal, but about general group by

Copy link
Contributor

@cloud-fan cloud-fan Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but now we do support star in aggregation, cc @rxin

Copy link
Member

@gatorsmile gatorsmile Aug 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the original discussion: #10731 (comment)

uh... one hour delay. I did not see @cloud-fan 's answer.

} else {
a.copy(aggregateExpressions = buildExpandedProjectList(a.aggregateExpressions, a.child))
}
Expand Down Expand Up @@ -723,9 +722,9 @@ class Analyzer(
if (index > 0 && index <= child.output.size) {
SortOrder(child.output(index - 1), direction)
} else {
throw new UnresolvedException(s,
s"Order/sort By position: $index does not exist " +
s"The Select List is indexed from 1 to ${child.output.size}")
s.failAnalysis(
s"ORDER BY position $index is not in select list " +
s"(valid range is [1, ${child.output.size}])")
}
case o => o
}
Expand All @@ -737,17 +736,18 @@ class Analyzer(
if conf.groupByOrdinal && aggs.forall(_.resolved) &&
groups.exists(IntegerIndex.unapply(_).nonEmpty) =>
val newGroups = groups.map {
case IntegerIndex(index) if index > 0 && index <= aggs.size =>
case ordinal @ IntegerIndex(index) if index > 0 && index <= aggs.size =>
aggs(index - 1) match {
case e if ResolveAggregateFunctions.containsAggregate(e) =>
throw new UnresolvedException(a,
s"Group by position: the '$index'th column in the select contains an " +
s"aggregate function: ${e.sql}. Aggregate functions are not allowed in GROUP BY")
ordinal.failAnalysis(
s"GROUP BY position $index is an aggregate function, and " +
"aggregate functions are not allowed in GROUP BY")
case o => o
}
case IntegerIndex(index) =>
throw new UnresolvedException(a,
s"Group by position: '$index' exceeds the size of the select list '${aggs.size}'.")
case ordinal @ IntegerIndex(index) =>
ordinal.failAnalysis(
s"GROUP BY position $index is not in select list " +
s"(valid range is [1, ${aggs.size}])")
case o => o
}
Aggregate(newGroups, aggs, child)
Expand Down