Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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

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

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

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
45 changes: 0 additions & 45 deletions sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -538,39 +538,6 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
sql("SELECT 1, 2, sum(b) FROM testData2"))
}

test("Group By Ordinal - negative cases") {
intercept[UnresolvedException[Aggregate]] {
sql("SELECT a, b FROM testData2 GROUP BY -1")
}

intercept[UnresolvedException[Aggregate]] {
sql("SELECT a, b FROM testData2 GROUP BY 3")
}

var e = intercept[UnresolvedException[Aggregate]](
sql("SELECT SUM(a) FROM testData2 GROUP BY 1"))
assert(e.getMessage contains
"Invalid call to Group by position: the '1'th column in the select contains " +
"an aggregate function")

e = intercept[UnresolvedException[Aggregate]](
sql("SELECT SUM(a) + 1 FROM testData2 GROUP BY 1"))
assert(e.getMessage contains
"Invalid call to Group by position: the '1'th column in the select contains " +
"an aggregate function")

var ae = intercept[AnalysisException](
sql("SELECT a, rand(0), sum(b) FROM testData2 GROUP BY a, 2"))
assert(ae.getMessage contains
"nondeterministic expression rand(0) should not appear in grouping expression")

ae = intercept[AnalysisException](
sql("SELECT * FROM testData2 GROUP BY a, b, 1"))
assert(ae.getMessage contains
"Group by position: star is not allowed to use in the select list " +
"when using ordinals in group by")
}

test("Group By Ordinal: spark.sql.groupByOrdinal=false") {
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, I know they will be tested in #14595 soon, but after we merge this PR and before merge #14595, this feature will be untested. Should we allow this? cc @rxin

Copy link
Contributor

Choose a reason for hiding this comment

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

How difficult is it to port the SQLQueryTestSuite harness back to branch-2.0?

withSQLConf(SQLConf.GROUP_BY_ORDINAL.key -> "false") {
// If spark.sql.groupByOrdinal=false, ignore the position number.
Expand Down Expand Up @@ -2467,18 +2434,6 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
Seq(Row(1, 1), Row(1, 2), Row(2, 1), Row(2, 2), Row(3, 1), Row(3, 2)))
}

test("order by ordinal number - negative cases") {
intercept[UnresolvedException[SortOrder]] {
sql("SELECT * FROM testData2 ORDER BY 0")
}
intercept[UnresolvedException[SortOrder]] {
sql("SELECT * FROM testData2 ORDER BY -1 DESC, b ASC")
}
intercept[UnresolvedException[SortOrder]] {
sql("SELECT * FROM testData2 ORDER BY 3 DESC, b ASC")
}
}

test("order by ordinal number with conf spark.sql.orderByOrdinal=false") {
withSQLConf(SQLConf.ORDER_BY_ORDINAL.key -> "false") {
// If spark.sql.orderByOrdinal=false, ignore the position number.
Expand Down