Skip to content

Conversation

@ahmed-mez
Copy link
Contributor

Rationale for this change

Fixes #552

What changes are included in this PR?

Make chunksToSingle concatenate multiple chunks into a single array using array.Concatenate instead of returning ErrNotImplemented

Are these changes tested?

Unit tests + tested end-to-end locally on a parquet file that contains chunked arrays

Are there any user-facing changes?

Changes are backward compatible

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Looks Good! Just two things: the comment below and can we add a round trip test that actually writes a file using a chunked table?

)

func TestChunksToSingle(t *testing.T) {
mem := memory.NewGoAllocator()
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a CheckedAllocator and assert the size to ensure we don't have any memory leak?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in dc63c09

@ahmed-mez
Copy link
Contributor Author

Thank you @zeroshade for the review!

I applied the CheckedAllocator suggestion and added a round trip test.

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Thanks much!!

@zeroshade zeroshade merged commit 3a01d9f into apache:main Oct 29, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[pqarrow] Support chunked array

2 participants