dongjoon-hyun commented on PR #58603:
URL: https://github.com/apache/spark/pull/58603#issuecomment-5584124344
Thanks for reworking this -- moving to the inference path is the right call,
and reverting the
parser-side option is the right thing to do given that parser recursion is
schema-bounded.
That said, I don't think the new code is reachable, and I believe the new
test passes on `master`
unchanged. Details below.
### 1. The new `StackOverflowError` handlers look like dead code, and the
test doesn't exercise them
`XMLInputFactory.newInstance()` in Spark resolves to **Woodstox**, shaded
into `hadoop-client`
(`StaxXMLRecordReader.scala:26` already imports
`org.apache.hadoop.shaded.com.ctc.wstx...`).
Woodstox enforces `maxElementDepth = 1000` by default, and
`StaxXmlParserUtils.factory` (line 38)
does not override it.
I reproduced this against the exact `hadoop-client-runtime` 3.5.0 jar that
`hadoop.version` in
`pom.xml` selects, with the same four factory properties Spark sets and the
exact document shape
from the new test (`<ROWS><ROW>` + `<a>` x 60000 + ...):
```
impl = org.apache.hadoop.shaded.com.ctc.wstx.stax.WstxInputFactory
events read = 1001
THROWN: javax.xml.stream.XMLStreamException : Maximum Element Depth limit
(1000) Exceeded
```
So the reader fails at depth 1001 with an `XMLStreamException`, long before
the JVM stack is
anywhere near exhausted -- `inferObject`/`inferField` never recurse deeply
enough to overflow.
And both catch sites already routed `XMLStreamException` through
`handleXmlErrorsByParseMode`
before this PR (`XmlInferSchema.scala:189` for the string path, `:303` for
the record-reader path).
That produces exactly the same error as the new `StackOverflowError` branch,
since
`malformedRecordsDetectedInSchemaInferenceError`
(`QueryExecutionErrors.scala:1568`) doesn't vary
with the cause:
```
MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION
badRecord -> _corrupt_record
failFastMode -> FAILFAST
```
which is precisely what the new test asserts. The test therefore can't
distinguish the two paths.
Could you run the two new tests against `master` without the
`XmlInferSchema` change? If they pass
there, this PR isn't doing what the description says.
(I verified the Woodstox behavior and the code paths directly; I did not run
`XmlSuite` itself.)
### 2. Catching `StackOverflowError` is unsafe regardless
Spark uses `NonFatal` consistently for a reason. A `StackOverflowError`
unwinds from an arbitrary
frame, so the Woodstox reader's internal state may be left mid-mutation;
continuing to touch it --
which `handleXmlErrorsByParseMode` does under `FAILFAST` via
`parser.close()` -- is not well
defined.
If we do want a bound on inference depth, I'd make it deterministic instead:
- set `WstxInputProperties.P_MAX_ELEMENT_DEPTH` explicitly on the factory,
which makes the limit
visible and adjustable and keeps it an ordinary `XMLStreamException` the
existing handlers
already cover; or
- add an explicit depth counter to `inferObject` / `inferField`.
Either is testable without depending on stack size.
### 3. The record-reader branch silently drops the rest of the file
`XmlInferSchema.scala:298` calls `parser.close()` and then returns a
corrupt-record schema. Closing
the streaming reader means **every remaining record in that file is excluded
from inference**. The
`XMLStreamException` branch above also closes, but that's a file that
genuinely can't be parsed any
further; here it's one deep record. Compare the `SAXException` branch right
below, which
deliberately does *not* close so the remaining records can still be inferred.
### 4. The test is environment-dependent even if the handler were reachable
`depth = 60000` depends on `-Xss4m` (the `sql/core` default -- note
`sql/hive` overrides it to
`-Xss64m`) and on the frame sizes of `inferObject`/`inferField`. Tests that
rely on triggering a
real stack overflow tend to be fragile across JDKs and JIT states. A
deterministic bound avoids
this entirely.
### 5. The PERMISSIVE assertion only checks the inferred schema
```scala
assert(df.columns.contains("_corrupt_record"))
```
This stops at inference. Adding a `df.collect()` / `checkAnswer` would
confirm the read actually
completes and show what the parse side produces. The description also
mentions `DROPMALFORMED`,
but there's no test for it.
### 6. Document-driven recursion on the parse side is still unbounded
`StaxXmlParser.convertVariantInternal` (line 1134) recurses per document
element rather than per
schema level, so `singleVariantColumn` or a `VariantType` field is
data-driven at parse time too.
Woodstox's 1000-element cap happens to shield it today, which is another
reason to make that cap
explicit rather than incidental.
---
Item (1) is the one to settle first: if the new tests pass on `master`, the
fix and the test both
need rethinking, and the real question becomes whether Woodstox's default
limit of 1000 should be
made explicit and configurable.
--
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]
