[SPARK-40416][SQL] Move subquery expression CheckAnalysis error messages to use the new error framework #37840
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @MaxGekk @srielau can you please help review this? 🙏 it helps make error messages better with respect to subquery expression analysis. @allisonwang-db FYI |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
|
Can one of the admins verify this patch? |
MaxGekk
reviewed
Sep 10, 2022
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
fetch latest from master
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
srielau
reviewed
Sep 12, 2022
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
srielau
reviewed
Sep 12, 2022
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
stash respond to code review comments stash respond to code review comments update tests
dtenedor
commented
Sep 12, 2022
There was a problem hiding this comment.
Thank you all @allisonwang-db @srielau @MaxGekk @gengliangwang for your helpful review. I responded to all comments, please take another look. Hat-tip to Gengliang with a helpful suggestion to add an overload expression.failAnalysis to propagate query contexts while adding the new error classes and minimizing diffs in CheckAnalysis.
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
dtenedor
requested review from
gengliangwang and
srielau
and removed request for
gengliangwang
dtenedor
changed the title
respond to code review comments respond to code review comments update update update update
fetch latest changes from master
MaxGekk
reviewed
Sep 14, 2022
sql/core/src/test/resources/sql-tests/results/udf/udf-except.sql.out
Outdated
Show resolved
Hide resolved
...ore/src/test/resources/sql-tests/results/subquery/negative-cases/invalid-correlation.sql.out
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala
Show resolved
Hide resolved
@gengliangwang thanks, this is done. We can see the new query contexts in this PR at the latest commit, they appear more accurate now. |
dtenedor
requested review from
allisonwang-db,
srielau,
gengliangwang and
MaxGekk
and removed request for
allisonwang-db,
srielau and
gengliangwang
|
Hi @MaxGekk I responded to your comments, please take another look at this when you have time. 🙏 @gengliangwang @allisonwang-db @srielau FYI |
sql/core/src/test/resources/sql-tests/results/join-lateral.sql.out
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
|
Friendly ping @MaxGekk @allisonwang-db @gengliangwang can we please review this again? |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/package.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala
Outdated
Show resolved
Hide resolved
|
LGTM except one comment |
respond to code review comments update update update simplify reuse existing plan canonicalize method respond to code review comments
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
|
Thanks, merging to master |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Move subquery expression CheckAnalysis error messages to use the new error framework.
This will help improve the usability of Apache Spark as a product, and link against documentation.
Why are the changes needed?
Error messages related to SQL query analysis in Apache Spark need some work to make them more descriptive and actionable.
Does this PR introduce any user-facing change?
Yes, error messages change.
How was this patch tested?
Unit test and query test coverage shows the updates in error messages received.