Skip to content

Conversation

@petermaxlee
Copy link
Contributor

@petermaxlee petermaxlee commented Aug 11, 2016

What changes were proposed in this pull request?

This patch improves error handling for group-by/order-by ordinals:

  1. Report error position in more places.
  2. Throw AnalysisException instead of UnresolvedException, which is an internal exception not meant for end-users.
  3. Made error messages more consistent.

Error messages look like the following:

org.apache.spark.sql.AnalysisException
GROUP BY position 3 is an aggregate function, and aggregate functions are not allowed in GROUP BY; line 1 pos 39
Star (*) is not allowed in select list when GROUP BY ordinal position is used;
org.apache.spark.sql.AnalysisException
ORDER BY position 3 is not in select list (valid range is [1, 2]); line 1 pos 28

How was this patch tested?

This is tested in SPARK-17015 but I'm submitting this part in isolation so it is easier to back port into branch-2.0. I will submit a separate pull request for SPARK-17015.

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.

@petermaxlee
Copy link
Contributor Author

Everything is tested here: #14595

@rxin
Copy link
Contributor

rxin commented Aug 11, 2016

LGTM pending Jenkins.

@SparkQA
Copy link

SparkQA commented Aug 11, 2016

Test build #63590 has finished for PR 14594 at commit 3e1b6b3.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

sql("SELECT 1, 2, sum(b) FROM testData2"))
}

test("Group By Ordinal - negative cases") {
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?

asfgit pushed a commit that referenced this pull request Aug 11, 2016
## What changes were proposed in this pull request?
This patch adds three test files:
1. arithmetic.sql.out
2. order-by-ordinal.sql
3. group-by-ordinal.sql

This includes #14594.

## How was this patch tested?
This is a test case change.

Author: petermaxlee <[email protected]>

Closes #14595 from petermaxlee/SPARK-17015.
@SparkQA
Copy link

SparkQA commented Aug 11, 2016

Test build #3217 has finished for PR 14594 at commit 156d5d8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 11, 2016

Test build #63594 has finished for PR 14594 at commit 156d5d8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request Aug 11, 2016
This patch adds three test files:
1. arithmetic.sql.out
2. order-by-ordinal.sql
3. group-by-ordinal.sql

This includes #14594.

This is a test case change.

Author: petermaxlee <[email protected]>

Closes #14595 from petermaxlee/SPARK-17015.

(cherry picked from commit a7b02db)
Signed-off-by: Reynold Xin <[email protected]>
@petermaxlee
Copy link
Contributor Author

Closing this since #14595 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants