LuciferYang commented on PR #58411: URL: https://github.com/apache/spark/pull/58411#issuecomment-5467883593
Thanks @peter-toth. Finding 2 is fixed and finding 1 took a gate with it. **1.** Confirmed, and the module argument in my description was wrong: `AvroFileFormat` and `AvroOptions` are both `private[sql]` in `sql/core`, so this object can name them. `hasProjectionSensitiveParser` now takes the options and answers true for avro under `positionalFieldMatching`, read off the map rather than through `AvroOptions`, whose constructor resolves `avroSchemaUrl` and would do I/O here, and read leniently so a malformed value still fails where Avro reports it rather than here. The test is in `AvroV1Suite`, because `avro` does not resolve from the `sql/core` test classpath. SPARK-59108 (#58409) removes the sensitivity at the root, and then this case and #58340's `AvroTable` gate come off together. **2.** Added to the paragraph that enumerates the conditions, naming the three parsers, avro under the option, and both strictness flags. **3.** Both numbers were arithmetic rather than measured: I added the seventeenth test and adjusted the counts instead of re-running. Re-measured, 11 of the 17 fail and 6 pass, and the sentence about shapes that keep merging now names the six it means. A pass over the rest of the description turned up more of the same class, all corrected: the table legend collided with its own header, the FAILFAST shape is CSV-specific rather than one of three, the mutation on the cached record fails two tests rather than one (`[14, 8, 88]` and `[14, 8, 88, 4]`), and the enumeration of the suite accounted for 15 of its 17 tests. **4.** I kept the class match. `hs.toString` is the registered short name and falls back to `HadoopFiles` for a format without a `DataSourceRegister`, so a list keyed on it fails silently in the direction that returns wrong rows, while a missing class case cannot be a typo. With avro named, every in-tree projection-sensitive format is covered, and a third-party one is covered by neither. If you would rather have the configuration so that a user can add one, I will take it. **5.** Gone. **6.** Computed once at the caller and passed in. **7.** `val` rather than `def`, which also stops `options` being retained as a field, so the serialized RDD is back to what master ships plus one boolean. **8.** Fixed. **9.** Fixed, with your sentence from #58340 carried over. Locally: the `planmerging` suites, `SubquerySuite`, `ExplainSuite`, `ExplainSuiteAE`, `FileBasedDataSourceSuite` and `InMemoryColumnarQuerySuite`, 423 tests; the whole `avro` module, 487; `catalyst/scalastyle`, `sql/scalastyle`, `sql/Test/scalastyle` and `avro/Test/scalastyle`; and the four mutation checks re-measured rather than inferred. -- 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]
