andygrove commented on PR #5513: URL: https://github.com/apache/datafusion-comet/pull/5513#issuecomment-5455716563
Thanks for this @pingzh, and sorry to arrive after the merge. I did a local pass over the merged commit and wanted to record what I found rather than let it sit in my notes. The good news first, because it is the part I most wanted to check. The data path holds up. I wrote round-trip probes for the cases the suite does not cover, nullable sliced `List`, `LargeList`, `Map`, `FixedSizeList` and `Struct` with dictionaries nested inside them, plus `NaN`, `-0.0`, positive and negative infinity, subnormals, empty strings, composed versus decomposed UTF-8, astral plane characters and embedded NULs. Every one round-trips exactly, so the dictionary garbage collection, the offset rebasing and the row-aligned splitting all preserve values and row order. I also verified every reflected Celeborn member against the real v0.6.0 and v0.7.0 sources and they all resolve, including the four-argument versus five-argument `mapperEnd` split. Locally I get 77 Rust tests and 128 JVM tests green on Spark 3.5 with Scala 2.12, and no encode regression at Spark's default `spark.shuffle.compress=true`. I did find four things worth following up on, so I filed them separately rather than burying them here: - #5527, the two new configs cannot both hold at their shipped defaults. `maxFrameBytes` defaults to 64 MiB but the effective per-row cap at the default 256 MiB `maxInFlightBytes` is about 36 MiB, and a row above that fails the query outright after four stage reruns. - #5528, the tracker assigns four fields that are `private final` in real Celeborn, and the visibility gap fails toward releasing admission while a payload is still in flight. - #5529, the bootstrap hook is fatal to `createClient` and is never uninstalled, so a Comet-specific incompatibility can stop all Celeborn shuffle on the executor. - #5530, Celeborn is not a build dependency in any scope, so the reflection is only ever exercised against hand-written stand-ins. #5527 is the one I would prioritise, since it has a short reproducer and affects anyone running the defaults. There are also some smaller perf and polish items I noticed that I did not think justified their own issues, and I am happy to add them to #5352 as a checklist if that is useful. -- 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]
