parthchandra commented on PR #6784:
URL:
https://github.com/apache/datafusion-comet/pull/6784#issuecomment-6350920280
> Thanks for the quick work on the earlier rounds, Parth. I went back
through every thread against `6f21c0dd39`, and the compression shim,
`line.maxlength`, the size ceiling, the per-read Hadoop conf, the ABFS decline
and the new tests all hold up. CI is green on this head across all five Spark
profiles. I also fuzzed the self-overlapping `lineSep` fallback against
Hadoop's `LineReader` with 200k random inputs per separator and buffer sizes
from 1 to 8 bytes. Separators without self-overlap matched an exact scan every
time, and `aab` mismatched about 8,400 times, so the fallback covers the right
class.
>
> I'm keeping this at request changes for one case where the native scan
returns different rows from Spark without an error.
>
> Spark's `HadoopLineRecordReader.skipUtfByteOrderMark` strips a leading BOM
from the first record whatever the separator is. The fix for the `lineSep=BOM`
case now skips the strip for every custom separator (`strip_leading_bom` in
`text_scan.rs`). I ran Hadoop 3.4.2's `LineRecordReader` on a few files to
compare:
>
> * `BOM a|b` with `lineSep="|"`: Hadoop returns `a`, `b`. The native scan
returns `BOM+a`, `b`.
> * A file holding only the BOM with `lineSep="|"`: Hadoop returns no rows.
The native scan returns one row containing U+FEFF.
> * `BOM|x` with `lineSep="|"`: Hadoop returns an empty row and `x`. The
native scan returns `BOM` and `x`.
>
> These files pass every eligibility check, so counts, filters and joins
over them would silently differ. I checked this against Hadoop's reader and
Spark 4.1.3's source, not end to end through Comet.
>
> Would it work to split first and then strip the BOM from the first record
only, as Hadoop does? If that record is exactly the BOM with no terminator
after it, it would produce no row. That also gives the right answer for
`lineSep=BOM` and for the default separator. Could you add SQL tests for `BOM
a|b` and for a BOM-only file, both with `lineSep="|"`? The existing BOM tests
only cover the default separator and `lineSep=BOM`.
>
> Two small notes on the description. The CSV node now builds its Hadoop
conf from the read options and no longer prints the protobuf in `stringArgs`,
which is the right change but isn't mentioned. And the multi-bucket reply
mentions a `crossLocations` set that isn't in the diff. The behavior is still
right because `splitPartitionsByStore` keys on the bucket, so I consider that
thread resolved.
>
> Once the BOM handling is fixed and tested I expect to approve.
Fixed. The reader now splits first and strips the BOM from the first record
only, like Hadoop's skipUtfByteOrderMark. A BOM-only file gives no rows, BOM|x
gives "" and x, and lineSep=BOM is unchanged. Added Rust and Scala tests for
BOM a|b, BOM|x and a BOM-only file with lineSep="|".
Also, updated the description.
--
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]
