gabotechs commented on code in PR #10044:
URL: https://github.com/apache/arrow-rs/pull/10044#discussion_r3371234776
##########
arrow-ipc/src/writer.rs:
##########
@@ -1808,13 +1941,15 @@ fn write_array_data(
array_data: &ArrayData,
buffers: &mut Vec<crate::Buffer>,
arrow_data: &mut Vec<u8>,
+ encoded_buffers: &mut Vec<EncodedBuffer>,
nodes: &mut Vec<crate::FieldNode>,
offset: i64,
num_rows: usize,
null_count: usize,
compression_codec: Option<CompressionCodec>,
compression_context: &mut CompressionContext,
write_options: &IpcWriteOptions,
+ is_direct: bool,
) -> Result<i64, ArrowError> {
let mut offset = offset;
Review Comment:
This is another example of my comment above, but this time reinforced by a
big Clippy bypass at the top.
Ideally, we should not be contributing towards stronger Clippy violations.
If you try to follow the steps above, there are good chances that we can either
maintain the width of this function's signature, or even reduce it.
##########
arrow-ipc/src/writer.rs:
##########
@@ -595,16 +700,47 @@ impl IpcDataGenerator {
let mut message = crate::MessageBuilder::new(&mut fbb);
message.add_version(write_options.metadata_version);
message.add_header_type(crate::MessageHeader::RecordBatch);
- message.add_bodyLength(arrow_data.len() as i64);
+ message.add_bodyLength(body_len as i64);
message.add_header(root);
let root = message.finish();
fbb.finish(root, None);
- let finished_data = fbb.finished_data();
+ let ipc_message = fbb.finished_data().to_vec();
- Ok(EncodedData {
- ipc_message: finished_data.to_vec(),
- arrow_data,
- })
+ if let Some(w) = writer {
Review Comment:
We need to find another way of modeling this without falling into
"if-driven-development" patterns.
Whenever you find yourself coding something switching execution branches
with completely different bodies over booleans, it's an indicator that you are
falling into an "if-driven-development" pattern, and this is will greatly hurt
maintenance in the future.
One way of trying to improve this situation can be:
1. Refactor things preferring code duplication, and split into different
functions with different responsibilities, even if that implies copy-pasting
big quantities of code.
2. Once you have the different functions with a fair amount of copy-pasted
code, try to see what are the common bits, and factor them out little by
little, trying to progressively reduce the LOC count in each one.
3. If you still see functions that need to accept `bool` or `Option`
parameters that have the capability of switching how the function behaves, that
means the function was the wrong abstraction on the first place, so don't be
afraid of tearing existing functions apart if that allows you to reuse smaller
bits in different places without switching on if statements.
--
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]
