Skip to content

Conversation

@timsaucer
Copy link
Member

@timsaucer timsaucer commented Mar 26, 2025


Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

* bugfix: correct offsets when serializing a list of fixed sized list and non-zero start offset (#7318)

* When serializing fixed length arrays, adjust the offsets for writing out

* Add unit test

* clippy warnings

* Add unit test for nulls

* Update unit test to account for which schema had nulls

* Add missing type annotation (#7326)

* Update version

* Create changelog

---------

Co-authored-by: Tim Saucer <[email protected]>
@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Mar 26, 2025
@timsaucer
Copy link
Member Author

@alamb this is the requested PR to merge the changelog back into main

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @timsaucer

write_options,
)?;
return Ok(offset);
} else if let DataType::FixedSizeList(_, fixed_size) = data_type {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is strange that github is showing this in the diff. However, I verified this code is already on main so this PR doesn't change the cide

} else if let DataType::FixedSizeList(_, fixed_size) = data_type {
assert_eq!(array_data.child_data().len(), 1);
let fixed_size = *fixed_size as usize;
let child_offset = array_data.offset() * fixed_size;
let child_length = array_data.len() * fixed_size;
let child_data = array_data.child_data()[0].slice(child_offset, child_length);
offset = write_array_data(
&child_data,
buffers,
arrow_data,
nodes,
offset,
child_data.len(),
child_data.null_count(),
compression_codec,
write_options,
)?;
return Ok(offset);
} else {

🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in fact the commit in main 0e73524 does not have these changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOH! My original PR never was merged into main! It was approved, but not merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now, via this.

@alamb alamb merged commit 0e73524 into main Mar 27, 2025
31 checks passed
@alamb
Copy link
Contributor

alamb commented Mar 27, 2025

DOH! My original PR never was merged into main! It was approved, but not merged.

No it was merged I just doubled checked 😅

Screenshot 2025-03-27 at 4 39 46 PM

The fact it showed up here in this PR I think is some sort of github rendering artifact

@alamb alamb deleted the 54.3.0_maintenance branch March 27, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate parquet Changes to the parquet crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants