MaxGekk opened a new pull request, #58836:
URL: https://github.com/apache/spark/pull/58836
### What changes were proposed in this pull request?
Make `PostProcessor.replaceTokenByIdentifier` in the shared SQL parser
rewrite a rule context only when its parent still holds that context as its
last child, which is the state every normal exit of `quotedIdentifier`,
`backQuotedIdentifier` and `nonReserved` is in. In any other state the listener
leaves the tree alone and lets whatever is unwinding the parser propagate.
ANTLR's generated rule methods call `exitRule` from a `finally` block, so
parse listeners also run while a `StackOverflowError` unwinds the parser. If
the overflow strikes inside `replaceTokenByIdentifier` itself, after
`parent.removeLastChild()` has detached the context and before
`parent.addChild` has attached the replacement token, the parser's current
context is left pointing at that context, and every enclosing rule's `finally`
calls `exitRule` on it again. The second run finds the parent empty,
`ArrayList.remove(-1)` throws, and that exception replaces the original error
on the way out:
```
java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 0
at java.util.ArrayList.remove(ArrayList.java:552)
at
org.antlr.v4.runtime.ParserRuleContext.removeLastChild(ParserRuleContext.java:196)
at
org.apache.spark.sql.catalyst.parser.PostProcessor$.replaceTokenByIdentifier(parsers.scala:343)
at
org.apache.spark.sql.catalyst.parser.PostProcessor$.exitQuotedIdentifier(parsers.scala:312)
at
org.apache.spark.sql.catalyst.parser.SqlBaseParser$QuotedIdentifierContext.exitRule(SqlBaseParser.java:36942)
at org.antlr.v4.runtime.Parser.exitRule(Parser.java:642)
at
org.apache.spark.sql.catalyst.parser.SqlBaseParser.singleDataType(SqlBaseParser.java:2889)
```
With the guard, the second run finds the parent no longer holding the
context and returns, and the `StackOverflowError` reaches the caller that
expects it. On a normal exit the parent's last child is always the context
being exited, so the guard changes nothing on the success path. A guard on the
context's own children alone is not enough, and was tried first: the context
still holds its matched token when the listener runs the second time; it is the
parent that has changed.
### Why are the changes needed?
`SchemaConverters.parseCatalystType` in the Avro connector converts a
`StackOverflowError` from a pathologically nested Catalyst type into an
`IncompatibleSchemaException` naming the property it came from (SPARK-59311).
On Java 25 the map-key case of that conversion never fires, because the
overflow lands inside `quotedIdentifier` and the listener above turns it into
an `IndexOutOfBoundsException`. The test `AvroSchemaHelperSuite."SPARK-59311: a
pathologically nested map-key type names the map-key property on overflow"`
therefore fails on every Java 25 run of `build_java25.yml` on master since the
SPARK-59311 follow-up, while the Java 17 job passes. Where the overflow lands
depends on frame sizes, so the same replacement can happen at any JDK to any
caller of the parser that is prepared for a `StackOverflowError`; the Avro test
is where it happened to become visible.
### Does this PR introduce _any_ user-facing change?
No. A parse that overflows the stack now surfaces the overflow itself, or
whatever the caller converts it into, instead of an unrelated
`IndexOutOfBoundsException` from inside the parser.
### How was this patch tested?
The existing `AvroSchemaHelperSuite` on JDK 25 (openjdk 25.0.4, x86_64),
where the map-key test failed three runs out of three before the change and the
whole suite passes with it:
```
build/sbt "avro/testOnly *AvroSchemaHelperSuite"
Tests: succeeded 10, failed 0, canceled 0, ignored 0, pending 0
```
The failure is order dependent, which is why it looked deterministic in CI:
the test passes when run alone on JDK 25 and fails after its sibling test has
warmed the parser with the same thousand-deep parse, which is the order the
suite always runs in. The suite is run whole for that reason. The Catalyst
parser suites (`DataTypeParserSuite`, `ExpressionParserSuite`,
`DDLParserSuite`, `PlanParserSuite`, `TableIdentifierParserSuite`,
`ErrorParserSuite`, `ParserUtilsSuite`) pass unchanged.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Fable 5.1)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
