Repository navigation
Conversation
|
Hi @cloud-fan and @sarutak, could you please take a look? Thanks. |
|
@XdithyX , thank you for fixing this issue. Please fix this issue after #54972 merges, because this issue was found during this PR. To keep this already big PR focused and fix it everywhere (including the code that is not part of #54972) I opened this Jira item (SPARK-56638) to track a follow up fix. |
|
CI has now passed. Please let me know if anything else is required. |
|
The goal of this JIRA item is to deprecate the usage of "_LEGACY_ERROR_TEMP_0064" in TABLESAMPLE code path and likely other code paths that depend on ParserUtils.scala. For example, in test("SPARK-55978: TABLESAMPLE SYSTEM - fraction out of range") in PlanParserSuite.scala. Starting from that call site, there is a call chain of -> AnalysisTest.scala -> AbstractSqlParser.scala -> AstBuilder.scala -> validate() function in ParserUtils.scala that is shared by both the existing Bernoulli sampling and the new System sampling. Fixing this will need code changes to 10+ call sites and 20+ test cases. That is a super set of TABLESAMPLE code path. Fixing them all at once is the best approach. I don't think your PR currently covers the full fix. |
|
@stanyao Thanks for clarifying. I originally scoped this PR to the TABLESAMPLE fraction validation path only, but I understand now that the expected fix is to eliminate I’ll update the PR to cover the full set of validate() call sites and their tests/golden files. |
99181d7 to
7b22383
Compare
|
Hi @stanyao I've updated the PR, can you please have a look when you get a chance? |
| ctx) | ||
| } | ||
|
|
||
| def invalidTableFunctionTableArgumentWithSinglePartitionError( |
There was a problem hiding this comment.
It's best to have test coverage in PlanParserSuite.scala for the newly introduced error classes here.
There was a problem hiding this comment.
@stanyao Thanks for pointing this out. I added PlanParserSuite coverage for the reachable table function parser error path, including the ORDER BY variant.
I also removed the two newly added table-function error classes that were not reachable from SQL parsing because the grammar rejects those forms earlier with PARSE_SYNTAX_ERROR.
|
Thanks for removing the entire ParserUtils.validate() and making progress of deprecating _LEGACY_ERROR_TEMP_0064! This PR touches the error framework (error-conditions.json, QueryParsingErrors), parser infrastructure (ParserUtils.validate removal, AstBuilder), and a bunch of golden files. Please get feedback from @cloud-fan and @MaxGekk. |
|
@stanyao Thanks for the review. @cloud-fan @MaxGekk Could you please have a look when you get a chance? |
stanyao
left a comment
There was a problem hiding this comment.
Overall looks good. I looped in 2 committers who are experts in this area for more feedback.
|
Hi @cloud-fan @MaxGekk Gentle follow up on this PR. |
|
cc @srielau |
|
BTW PR needs a rebase, |
|
Thanks @cloud-fan. Rebased the PR on latest master to pick up the Also re-ran the parser coverage after rebase: build/sbt 'catalyst / Test / testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite -- -z "Support PARTITION BY and ORDER BY clause for TVF TABLE arguments"' |
|
Gentle follow-up @srielau @cloud-fan . Please let me know if anything else is needed from my side. Thanks! |
|
Thank you @srielau for the review. @cloud-fan Can we check this in, if there are no other issues? |
|
thanks, merging to master/4.x! |
…tion failures ### What changes were proposed in this pull request? This PR removes the remaining use of `_LEGACY_ERROR_TEMP_0064` from parser validation paths that depended on `ParserUtils.validate(...)` ([SPARK-56638](https://issues.apache.org/jira/browse/SPARK-56638)). Specifically, this PR: - Adds named parser error conditions for the affected validation failures - Adds corresponding `QueryParsingErrors` helpers - Replaces `ParserUtils.validate(...)` call sites with specific parser errors - Removes `ParserUtils.validate(...)` - Updates parser tests and SQL golden files to assert the new error classes This includes the `TABLESAMPLE` fraction validation path, covering both existing `TABLESAMPLE` syntax and `TABLESAMPLE SYSTEM` from [#54972](#54972). ### Why are the changes needed? `ParserUtils.validate(...)` always threw `_LEGACY_ERROR_TEMP_0064`, which made different parser validation failures share the same generic legacy error class. Using named error classes makes each parser failure explicit and removes this legacy temporary error from the affected parser paths. `INVALID_TABLESAMPLE_FRACTION` uses SQLSTATE `22023` because the statement is syntactically valid, but the supplied sampling fraction is invalid/out of range. The other parser syntax validation errors are added under `INVALID_SQL_SYNTAX`, which uses SQLSTATE `42000`. ### Does this PR introduce _any_ user-facing change? Yes. The error class changes from `_LEGACY_ERROR_TEMP_0064` to named error classes for the affected parser validation failures. For invalid `TABLESAMPLE` fractions, the new error class is `INVALID_TABLESAMPLE_FRACTION`. ### How was this patch tested? ```bash build/sbt 'catalyst / Test / testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite -- -z "sampled relations"' build/sbt 'catalyst / Test / testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite -- -z "TABLESAMPLE SYSTEM - fraction out of range"' SPARK_GENERATE_GOLDEN_FILES=1 build/sbt 'sql / Test / testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z "tablesample-negative.sql"' SPARK_GENERATE_GOLDEN_FILES=1 build/sbt 'sql / Test / testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z "pipe-operators.sql"' build/sbt 'sql / Test / testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z "tablesample-negative.sql"' 'sql / Test / testOnly org.apache.spark.sql.SQLQueryTestSuite -- -z "pipe-operators.sql"' ``` ### Was this patch authored or co-authored using generative AI tooling? Yes. Generated-by: OpenAI GPT-5.5 Codex Closes #55782 from XdithyX/SPARK-56638. Authored-by: Adithya Ajith <4adhi007@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 447d3b4) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
|
Thank you @cloud-fan |
What changes were proposed in this pull request?
This PR removes the remaining use of
_LEGACY_ERROR_TEMP_0064from parser validation paths that depended onParserUtils.validate(...)(SPARK-56638).Specifically, this PR:
QueryParsingErrorshelpersParserUtils.validate(...)call sites with specific parser errorsParserUtils.validate(...)This includes the
TABLESAMPLEfraction validation path, covering both existingTABLESAMPLEsyntax andTABLESAMPLE SYSTEMfrom #54972.Why are the changes needed?
ParserUtils.validate(...)always threw_LEGACY_ERROR_TEMP_0064, which made different parser validation failures share the same generic legacy error class.Using named error classes makes each parser failure explicit and removes this legacy temporary error from the affected parser paths.
INVALID_TABLESAMPLE_FRACTIONuses SQLSTATE22023because the statement is syntactically valid, but the supplied sampling fraction is invalid/out of range. The other parser syntax validation errors are added underINVALID_SQL_SYNTAX, which uses SQLSTATE42000.Does this PR introduce any user-facing change?
Yes. The error class changes from
_LEGACY_ERROR_TEMP_0064to named error classes for the affected parser validation failures.For invalid
TABLESAMPLEfractions, the new error class isINVALID_TABLESAMPLE_FRACTION.How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
Yes. Generated-by: OpenAI GPT-5.5 Codex