andygrove commented on PR #2204:
URL:
https://github.com/apache/datafusion-ballista/pull/2204#issuecomment-5226637163
Heads up that CI is red here on three jobs (`macos test`, `test linux
ballista`, `test linux crates`), and I think it's real rather than flaky.
`should_force_local_read::case_1_standalone` and `case_2_remote` fail with
`IpcError("Premature end of the stream while decoding schema")`. That traces
back to the block transfer path, which I don't think the format switch covers.
`flight_service.rs` around line 340 opens the shuffle file with
`tokio::fs::File` and ships the raw bytes over `ReaderStream` for the
`do_action` block transfer from #1315, and the client decodes those bytes with
`StreamDecoder` in `client.rs`. `StreamDecoder` only speaks stream format, so
once `write_stream_to_disk` emits file format the bytes lead with the `ARROW1`
magic and trail a footer, and `BlockDataStream::try_new` never finds a schema.
That one isn't a type swap unfortunately, since raw block transfer is
inherently stream shaped. The client would need to skip the magic and footer,
or use a different decoder.
Related, and probably worth fixing in the same pass: I think the writer side
is only half migrated. `write_stream_to_disk` covers the `output_partitioning:
None` branch of `ShuffleWriterExec`, but the hash repartition branch builds its
own writer inline around `shuffle_writer.rs:325` and still uses `StreamWriter`,
while both readers moved to `FileReader`. That path would write stream and read
file. It isn't failing in CI today because sort shuffle is the default and has
its own untouched writer and reader, which is also why TPC-H SF10 stayed green.
One thing that made both of these easy to miss: the three updated tests in
`shuffle_reader.rs` fabricate their files with `FileWriter` directly, so they
only assert the reader agrees with itself. A round trip that drives
`ShuffleWriterExec` through both branches into the reader, plus one exercising
forced remote block transfer, would cover the gap.
I have more feedback on the rest of the PR but will hold it until CI is
green.
--
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]
