dongjoon-hyun commented on PR #58603:
URL: https://github.com/apache/spark/pull/58603#issuecomment-5581156579
Thanks for the PR. I checked out the branch and read the surrounding parser
code. The overall
direction (opt-in option, unlimited by default) is fine, but there are a few
things I'd like to
resolve before this goes in.
### 1. `depth` is not propagated on three recursion paths, so the limit can
be bypassed
`StaxXmlParser.scala` lines 391, 515 and 523 call `convertField` without
passing `depth`, so it
falls back to the default `1` and the counter resets:
```scala
// 523: in convertObject. `dt` here is whatever is not Struct/Array/Variant
-- i.e. MapType
case dt: DataType => row(index) = convertField(parser, dt, field,
attributes) // depth = 1
// 391: in convertMap
kvPairs += (UTF8String.fromString(key) -> convertField(parser, valueType,
key)) // depth = 1
// 515: in the ArrayType(dt) branch, when dt is itself ArrayType(...) /
MapType(...)
case dt: DataType => convertField(parser, dt, field)
// depth = 1
```
Concrete case -- `maxNestingDepth=2` with schema
`struct<m: map<string, struct<a: struct<b: struct<c: string>>>>>`: field `m`
goes 523 ->
`convertMap` -> 391, the depth resets to 1, and the three nested structs
below it are never
checked. `array<array<struct<...>>>` bypasses it the same way through 515.
The `depth: Int = 1` default parameter is what makes this easy to get wrong.
I'd drop the default
on `convertField` / `convertObject` / `convertObjectWithAttributes` and make
it a required
parameter, passing `1` only at the two entry points (lines 160 and 245) --
then the compiler
catches any missed call site. `convertMap` needs to take and forward `depth`
too.
### 2. The stated motivation doesn't match where the recursion actually is
Recursion in `convertObject` is driven by the **schema**, not by the
document. Subtrees not present
in the schema go through `skipChildren`, and `StringType` fields through
`currentStructureAsString` -- both iterative. So parsing a very deeply
nested document against a
shallow schema never recurses deeply, and this option is effectively a
per-record re-check of the
schema nesting depth.
The place that does recurse per document element, without any bound, is
`XmlInferSchema.inferObject` / `inferField`, which this PR doesn't touch.
That means the common
case -- `spark.read.xml(path)` with schema inference -- still overflows the
stack during inference
before the new limit can ever apply.
If bounding parser stack depth is the goal, inference needs the same
treatment. If not, could we
reword the motivation in the PR description to match what the option
actually does?
### 3. The subtree isn't consumed before throwing, which desynchronizes the
event stream
The `throw` at line 475 happens before any of the offending element's child
events are consumed.
That exception is caught by the parent `convertObject`'s `case NonFatal(e)`
handler (line 547) and
the loop keeps going, so the remaining child `StartElement`s get interpreted
as fields of the
*parent* struct and the child's `</...>` is taken as the parent's
`EndElement`, ending that level
early.
The record becomes a bad record via `PartialResultException` either way, so
no wrong values are
returned. But the optimized path (`doParseColumnOptimized`) resynchronizes
with
`skipToNextRowStart`, so for a document with a **nested `rowTag`** -- which
is exactly the kind of
recursive XML this option targets -- the leftover events can be picked up as
a spurious extra
record. Skipping the subtree (`StaxXmlParserUtils.skipChildren(parser,
field, options)`) before
throwing, or doing the check in the parent before descending, would keep the
stream in sync.
### 4. Please use the error class framework
```scala
throw new IllegalStateException(
s"XML element nesting depth exceeds the configured maximum of ...")
```
New user-facing errors should go through an error condition in
`error-conditions.json`; line 363 of
the same file already uses `SparkIllegalArgumentException(errorClass =
...)`. Also, the trailing
`(${XmlOptions.MAX_NESTING_DEPTH})` renders as the literal option *name*
`(maxNestingDepth)`, which
reads ambiguously -- something like "set via the `maxNestingDepth` option"
would be clearer.
### 5. No validation on the option value
```scala
val maxNestingDepth =
parameters.get(MAX_NESTING_DEPTH).map(_.toInt).getOrElse(-1)
```
A non-numeric value produces a bare `NumberFormatException` with no context,
and `0` or `-5`
silently mean "unlimited" even though the comment says only `-1` does.
`samplingRatio` right above
it (`XmlOptions.scala:112`) uses `require(...)`; the same here would be good.
### 6. No tests
This adds a user-facing option with no test coverage --
"`XmlExpressionsSuite` continues to pass"
is a regression check, not a test of the feature. At minimum I'd like to
see, in `XmlSuite`:
- a record exceeding the limit landing in the corrupt-record column under
PERMISSIVE **while the
following well-formed records still parse** (the behavior the description
claims)
- FAILFAST / DROPMALFORMED behavior
- no behavior change at the default (`-1`)
- the map / nested-array cases from (1), which should currently fail
### 7. Missing documentation and PySpark plumbing
- No entry in the option table in `docs/sql-data-sources-xml.md`; every
other XML option is listed
there.
- Not in the explicit kwargs of `DataFrameReader.xml`
(`python/pyspark/sql/readwriter.py`), nor the
Connect and streaming readers.
### Minor
`Generated-by: Isaac` -- the template expects the model/tool that was used,
so reviewers may ask
about this.
Items (1) and (6) look like blockers to me; (2) is worth settling first,
since it determines what
this option is really for.
--
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]
