andygrove commented on code in PR #5006:
URL: https://github.com/apache/datafusion-comet/pull/5006#discussion_r3641323175
##########
native/shuffle/src/writers/shuffle_block_writer.rs:
##########
@@ -16,12 +16,20 @@
// under the License.
use arrow::array::RecordBatch;
-use arrow::datatypes::Schema;
-use arrow::ipc::writer::StreamWriter;
+use arrow::datatypes::{DataType, Schema, SchemaRef};
+use arrow::ipc::writer::{
+ write_message, CompressionContext, DictionaryTracker, IpcDataGenerator,
IpcWriteOptions,
+ StreamWriter,
+};
use datafusion::common::DataFusionError;
use datafusion::error::Result;
use datafusion::physical_plan::metrics::Time;
-use std::io::{Cursor, Seek, SeekFrom, Write};
+use std::io::{Seek, SeekFrom, Write};
+use std::sync::Arc;
+
+/// Arrow IPC stream end-of-stream marker: the continuation marker
(`0xFFFFFFFF`) followed by a
+/// zero message length, matching what `StreamWriter::finish` emits for
metadata version V5.
+const IPC_EOS: [u8; 8] = [0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00];
Review Comment:
Took the intent here but not the literal form, since the suggested assert
does not compile: `IpcWriteOptions::metadata_version` is a private field
(arrow-ipc 58.3 `writer.rs:65`) and there is no public getter on
`IpcWriteOptions`, so it cannot be inspected from outside the crate.
Pinned V5 at the single construction site instead:
```rust
let write_options = IpcWriteOptions::try_new(64, false,
MetadataVersion::V5)?;
```
`(64, false, V5)` is exactly what `IpcWriteOptions::default()` builds
(`writer.rs:148-157`), so behavior is unchanged, but V5 becomes a deliberate
choice rather than an inherited default, and the constraint holds in release
builds rather than only under `debug_assertions`. Since that one value now
feeds every encode site, the options that produce the stream and the hardcoded
`IPC_EOS` cannot diverge.
Also expanded the `IPC_EOS` doc comment to state the V4-legacy contrast
explicitly (four zero bytes, no continuation marker) and to point at the pin as
the reason the constant is valid.
--
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]
