LuciferYang commented on PR #58409:
URL: https://github.com/apache/spark/pull/58409#issuecomment-5519611495
Thanks, both of you. `2ec57b9c2d1` retires the two gates and takes the two
comment fixes; one finding I think is refuted, with the measurement below.
**Retiring the gates (@peter-toth's 6, and cloud-fan's P2 on the same
thread).** Done in the order you gave. `AvroTable.supportsScanMerging` is an
unconditional `true` rather than a deleted override, since `FileTable` defaults
it to false; `hasProjectionSensitiveParser` loses its avro arm and with it the
`options` parameter and the `org.apache.spark.sql.avro` import; the
`AvroV1Suite` case from #58411 and the capability assertion in the
`AvroV2Suite` SPARK-57205 case are gone. Two things the compiler caught that
the list did not: `MergeSubplans` and `FileSourceScanExec` became unused
imports in `AvroSuite` with that test, and `AvroOptions` became one in
`AvroTable`, where the only mention left is prose. The documentation went with
them: `docs/sql-performance-tuning.md` no longer lists avro among the
projection-sensitive V1 relations, and the V2 paragraph no longer ends with
Avro withholding the capability. `FileTable`'s class doc keeps the shape as a
general statement of the contr
act, as you said.
**Duplicate names (cloud-fan's P2 at `AvroDeserializer.scala:478`) I believe
is refuted.** The premise is that Spark permits a duplicate-name file-source
schema, so I tried to build one on both read paths before writing the ordinal
plumbing:
```
V1, caseSensitive=false: AnalysisException [COLUMN_ALREADY_EXISTS] The
column `x` already exists
V2, caseSensitive=false: same
V1, caseSensitive=true: same
V2, caseSensitive=true: same
```
`spark.read.schema("x long, x long")` is rejected before the read on all
four combinations, so a duplicate-name data schema never reaches the
deserializer and `fieldIndex` has no two occurrences to collapse. The other
routes to one are closed too: the Avro spec forbids duplicate field names in a
record, so an inferred schema cannot have them; the `avroSchema` option
supplies the Avro side rather than the Catalyst data schema; and `x` beside `X`
is rejected by the same check when analysis is case-insensitive, while under
case-sensitive analysis they are two distinct names that `fieldIndex`
separates, which the mixed-case test already covers. If you would rather have
the state ruled out in code as well, I can add a uniqueness assertion where the
positions are computed, but I would rather not add a check for a state that
cannot arise.
**Nits.** Both taken: `that position is the one in the full schema rather
than in the projection`, and `With pushdown on, the filter runs inside the
deserializer; with it off, it runs above the scan.`
The description now says what this commit removes rather than what it will
have to, and names the two commits the prerequisites landed as. It does not
claim duplicate-name coverage, for the reason above. A backport carries the
same removal wherever both prerequisites are, which today is `master` and
`branch-4.x`.
--
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]
