fix(spark): throw proper ParseExceptions in the six extended SQL AST... - #19460
Conversation
…builders
On Spark 3.4+ the (String, ParserRuleContext) ParseException constructor
treats the string as an error class, so the builders' 28 raw throw sites
surfaced at runtime as SparkException [INTERNAL_ERROR] (or a bare
AssertionError for messages containing dots), and the three
construct-then-setStackTrace sites discarded the original cause. Route
all sites through a per-version parseException helper (message-based
constructor on 3.3-3.5, _LEGACY_ERROR_TEMP_0035 on 4.x where the message
constructor is private), fix the unbraced ${nonRef.describe}
interpolation, and tighten the invalid-partition-transform test to
intercept[ParseException].
Fixes apache#19450
voonhous
changed the title
voonhous
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19460 +/- ##
============================================
+ Coverage 76.81% 76.91% +0.09%
- Complexity 33770 33842 +72
============================================
Files 2576 2576
Lines 143457 143463 +6
Branches 17589 17589
============================================
+ Hits 110201 110338 +137
+ Misses 25010 24851 -159
- Partials 8246 8274 +28
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR fixes the six HoodieSpark*ExtendedSqlAstBuilders so extended-parser errors (BLOB/VECTOR DDL, index statements) surface as a clean ParseException across every supported Spark version (3.3–4.2), by routing all raw throw sites through a per-version parseException helper and correcting an unbraced ${nonRef.describe} interpolation. I verified against the actual PR head that all six builders route every throw site through the helper (no un-converted two-arg new ParseException(message, ctx) sites remain), that the 3.4/3.5 helper matches the pattern already used by the existing extended parsers, and that the 4.x helper mirrors Spark's own operationNotAllowedError (correct legacy error class and message parameter). No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
…4.x helper
Add a shared interceptParse helper that asserts a clean ParseException plus
a message substring; the previous checkExceptionContain assertions catch any
Throwable, and the pre-fix [INTERNAL_ERROR] text embeds the original message,
so they passed while every site was broken. Convert the negative parser
assertions in TestBlobDataType and TestCreateTable, add four cases for the
previously untested visitor arms (typed literals, invalid INTERVAL, the
out-of-range AssertionError mode, mixed interval fields), pin the
${nonRef.describe} rendering, strip the trailing period in the 4.x helper
to avoid double periods, document the cross-profile prefix and the
_LEGACY_ERROR_TEMP_0035 coupling, and mark the grammar-unreachable interval
arm for the next pruning pass.
|
Thanks for the review, let me address the comments and fix the coverage issues before merging. |
…view asserts Address review comments: pin the interpolation fix by asserting the absence of ".describe" in the column-reference message, and assert the restored setStackTrace by checking for an IntervalUtils frame on the invalid-INTERVAL case. Address the codecov patch-coverage gap with a new negative test covering seven more converted throw sites (unsupported typed literal, hex IllegalArgumentException fallback, both single-from-to-unit arms, non-numeric unit value, non-string from-to value, unsupported from-to pair); interceptParse now returns the ParseException for follow-on assertions. The remaining uncovered sites re-wrap Spark-utility exceptions whose types changed across 3.3-4.2 (e.g. over-precision decimals throw SparkArithmeticException on 4.x, bypassing the AnalysisException catch), are grammar-unreachable, or need ANTLR error-recovery trees.
|
Re the codecov patch-coverage gap: added
The remaining uncovered converted lines are intentionally left:
|
|
Thanks for the review and approval @wombatu-kun! Both comments are addressed in 2cbbd13:
The same commit closes most of the codecov patch gap (7 more throw sites covered, details in the comment above). Verified green locally on Spark 3.5 and 4.2; CI should confirm the rest of the matrix. |
|
Let's merge this in first given that the coverage is > 80% |
5 participants
Describe the issue this Pull Request addresses
Closes #19450
The six
HoodieSpark*ExtendedSqlAstBuilders throw parse errors asnew ParseException(<message>, ctx). On Spark 3.3 that constructor's String is the human message, but on Spark 3.4+ it is the error class, so every such site surfaces at runtime asSparkException: [INTERNAL_ERROR] Cannot find main error class '<msg>'instead of a cleanParseException. Messages containing 2+ dots (e.g. interpolatede.getMessage, or numeric ranges like-3.4028234663852886E+38) are even worse: they fail an assertion inside Spark'sErrorClassesJsonReaderand throw a bareAssertionErrorwith no diagnostic text at all. The three construct-then-setStackTracesites also lost the original exception's stack trace on 3.4+, because the constructor threw beforesetStackTracecould run (acausecannot be attached at all:ParseExceptionnever passes one toAnalysisException, andinitCauseis blocked once the null cause is committed).Summary and Changelog
Extended-parser errors (BLOB/VECTOR DDL, index statements) now surface as a clean
ParseExceptioncarrying the intended message on every supported Spark version.Production changes:
parseException(message, ctx)helper to each of the six builders (3.3, 3.4, 3.5, 4.0, 4.1, 4.2) and route all 28 raw throw sites per builder through it, keeping call-site text identical across the six files to preserve diffability:errorClassstaysNone), the same patternHoodieSpark3_4/3_5ExtendedSqlParseralready use.(errorClass, messageParameters, ctx)constructor with_LEGACY_ERROR_TEMP_0035, the same classParserUtils.operationNotAlloweduses, because the message-based constructor isprivateon Spark 4.x. A trailing period is stripped from the message so the template does not render a double period.setStackTraceof the original cause's stack trace, which previously never ran on 3.4+.$nonRef.describebecomes${nonRef.describe}(it printed the literal text.describe).at least one time unitinterval arm with a comment for the next pruning pass (a bareINTERVALkeyword binds totransformArgument'squalifiedNamealternative first).Test changes:
interceptParse(sql)(expected)helper toExtendedParserTestHelpersthat asserts a cleanParseExceptionplus a message substring. The previouscheckExceptionContain-based assertions could not detect this bug class: they catch anyThrowableand the pre-fix[INTERNAL_ERROR]text embeds the original message, so substring matching passed while every site was broken.TestBlobDataType(invalid partition transforms, interval endpoints) andTestCreateTable(VECTOR without dimension, invalid VECTOR type, duplicated table paths, reserved table properties) tointerceptParse.DATE 'nope'), invalidINTERVALliterals (thesetStackTracearm), out-of-range fractional literals (the pre-fixAssertionErrormode), and mixed year-month/day-time interval fields. Coverage of the 28 converted sites goes from 3 to 15.No code was copied; the builders themselves remain the pre-existing fork of Spark's AstBuilder.
Impact
No public API changes. User-facing error behavior on Spark 3.4+ profiles: extended-parser failures now raise
ParseExceptionwith the intended message instead ofSparkException [INTERNAL_ERROR]orAssertionError. On Spark 4.x profiles the message is rendered through the legacy error class asOperation not allowed: <message>.; on 3.3-3.5 messages are byte-identical to before. Known divergence from stock Spark: for the same SQL error, stock Spark answers with named error classes (e.g.INVALID_SQL_SYNTAX.INVALID_COLUMN_REFERENCE); thoseQueryParsingErrorshelpers take Spark's own parser-context types, so they cannot be reused from Hudi's forked grammar contexts. These statements are only reachable through Hudi's extended parser. No performance impact (error paths only).Risk Level
low. The change is confined to error-raising paths in the extended parsers. All six Spark profiles were compile-gated locally,
hudi-sparkwas test-compiled under spark3.5 (Scala 2.12) and spark4.2 (Scala 2.13), and the modified suites were executed against the Spark 4.2 classpath (the riskiest variant, exercising the_LEGACY_ERROR_TEMP_0035rendering end to end). If a future Spark release drops that legacy class, theinterceptParseassertions fail loudly on the 4.x CI profiles rather than silently regressing.Documentation Update
none
Contributor's checklist