Rich-T-kid commented on code in PR #10128:
URL: https://github.com/apache/arrow-rs/pull/10128#discussion_r3624622943
##########
arrow-ipc/src/compression.rs:
##########
@@ -84,6 +84,7 @@ impl std::fmt::Debug for IpcWriteContext {
}
/// Deprecated alias for [`IpcWriteContext`].
+#[expect(dead_code)]
Review Comment:
Why is this needed? on main this is used so there isnt any need for a
dead_code clippy ignore.
[here](https://github.com/apache/arrow-rs/blob/cd17899fe4c68c812a9585ccaf4a36f1d30e0978/arrow-ipc/src/compression.rs#L87)
##########
arrow-ipc/src/writer.rs:
##########
@@ -1890,7 +1954,7 @@ fn write_body_buffers<W: Write>(
/// Write a record batch to the writer, writing the message size before the
message
/// if the record batch is being written to a stream
-fn write_continuation<W: Write>(
+fn write_continuation_and_meta_size<W: Write>(
Review Comment:
Can we also update the comment above the function to match what its doing now
##########
arrow-ipc/src/writer.rs:
##########
@@ -1356,6 +1374,9 @@ pub struct FileWriter<W> {
data_gen: IpcDataGenerator,
+ /// Holds reusable scratch state shared across all messages -- including
the
+ /// [`FlatBufferBuilder`] whose internal buffer is reused to avoid
reallocating
+ /// on every batch.
Review Comment:
👍
##########
arrow-ipc/src/writer.rs:
##########
@@ -1836,41 +1885,56 @@ pub fn write_message<W: Write>(
));
}
+ let aligned_size = write_message_header(&mut writer, &encoded.ipc_message,
write_options)?;
+
+ // write arrow data
+ let body_len = if arrow_data_len > 0 {
+ write_body_buffer(&mut writer, &encoded.arrow_data,
write_options.alignment)?
+ } else {
+ 0
+ };
+
+ Ok((aligned_size, body_len))
+}
+
+/// Write the encapsulated-message header to `writer`: the continuation marker
and
+/// metadata length, the flatbuffer metadata (`ipc_message`), and the alignment
+/// padding that follows it.
+///
+/// Returns the padded header length (continuation prefix + metadata +
padding),
+/// which is also the value encoded in the continuation length field.
+fn write_message_header<W: Write>(
+ writer: &mut W,
+ ipc_message: &[u8],
+ write_options: &IpcWriteOptions,
+) -> Result<usize, ArrowError> {
let a = usize::from(write_options.alignment - 1);
- let buffer = encoded.ipc_message;
- let flatbuf_size = buffer.len();
+ let flatbuf_size = ipc_message.len();
let prefix_size = if write_options.write_legacy_ipc_format {
4
} else {
8
};
let aligned_size = (flatbuf_size + prefix_size + a) & !a;
- let padding_bytes = aligned_size - flatbuf_size - prefix_size;
- write_continuation(
- &mut writer,
+ write_continuation_and_meta_size(
+ &mut *writer,
write_options,
(aligned_size - prefix_size) as i32,
)?;
// write the flatbuf
if flatbuf_size > 0 {
- writer.write_all(&buffer)?;
+ writer.write_all(ipc_message)?;
}
- // write padding
- writer.write_all(&PADDING[..padding_bytes])?;
- // write arrow data
- let body_len = if arrow_data_len > 0 {
- write_body_buffers(&mut writer, &encoded.arrow_data,
write_options.alignment)?
- } else {
- 0
- };
+ // write padding
+ writer.write_all(&PADDING[..aligned_size - flatbuf_size - prefix_size])?;
Review Comment:
can we add a comment explaining the math here
##########
arrow-ipc/src/writer.rs:
##########
@@ -1890,7 +1954,7 @@ fn write_body_buffers<W: Write>(
/// Write a record batch to the writer, writing the message size before the
message
/// if the record batch is being written to a stream
-fn write_continuation<W: Write>(
+fn write_continuation_and_meta_size<W: Write>(
Review Comment:
on second thought, the function body didnt change besides hoisting the
`null_count()` function. why did the function name chage?
##########
arrow-ipc/src/writer.rs:
##########
@@ -1836,41 +1885,56 @@ pub fn write_message<W: Write>(
));
}
+ let aligned_size = write_message_header(&mut writer, &encoded.ipc_message,
write_options)?;
+
+ // write arrow data
+ let body_len = if arrow_data_len > 0 {
+ write_body_buffer(&mut writer, &encoded.arrow_data,
write_options.alignment)?
+ } else {
+ 0
+ };
+
+ Ok((aligned_size, body_len))
+}
+
+/// Write the encapsulated-message header to `writer`: the continuation marker
and
+/// metadata length, the flatbuffer metadata (`ipc_message`), and the alignment
+/// padding that follows it.
+///
+/// Returns the padded header length (continuation prefix + metadata +
padding),
+/// which is also the value encoded in the continuation length field.
+fn write_message_header<W: Write>(
+ writer: &mut W,
+ ipc_message: &[u8],
+ write_options: &IpcWriteOptions,
+) -> Result<usize, ArrowError> {
let a = usize::from(write_options.alignment - 1);
- let buffer = encoded.ipc_message;
- let flatbuf_size = buffer.len();
+ let flatbuf_size = ipc_message.len();
let prefix_size = if write_options.write_legacy_ipc_format {
4
} else {
8
};
let aligned_size = (flatbuf_size + prefix_size + a) & !a;
- let padding_bytes = aligned_size - flatbuf_size - prefix_size;
- write_continuation(
- &mut writer,
+ write_continuation_and_meta_size(
+ &mut *writer,
write_options,
(aligned_size - prefix_size) as i32,
)?;
// write the flatbuf
if flatbuf_size > 0 {
- writer.write_all(&buffer)?;
+ writer.write_all(ipc_message)?;
}
- // write padding
- writer.write_all(&PADDING[..padding_bytes])?;
- // write arrow data
- let body_len = if arrow_data_len > 0 {
- write_body_buffers(&mut writer, &encoded.arrow_data,
write_options.alignment)?
- } else {
- 0
- };
+ // write padding
+ writer.write_all(&PADDING[..aligned_size - flatbuf_size - prefix_size])?;
- Ok((aligned_size, body_len))
+ Ok(aligned_size)
}
-fn write_body_buffers<W: Write>(
+fn write_body_buffer<W: Write>(
Review Comment:
can we try to avoid changes like this? removing the `s` makes it seem like
more LOC changed then their really were.
--
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]
