Skip to content

feat: impl BatchCoalescer::push_batch_with_indices#8991

Merged
alamb merged 1 commit intoapache:mainfrom
ClSlaid:feat/batch-coalescer/push-batch-with-indices
Dec 15, 2025
Merged

feat: impl BatchCoalescer::push_batch_with_indices#8991
alamb merged 1 commit intoapache:mainfrom
ClSlaid:feat/batch-coalescer/push-batch-with-indices

Conversation

@ClSlaid
Copy link
Copy Markdown
Contributor

@ClSlaid ClSlaid commented Dec 14, 2025

MVP for #8957
awaits for #8951

very first version for reviewers to confirm behaviour, optimizations TBD

MVP for apache#8957
awaits for apache#8951

very first version for behaviour review, optimizations TBD

Signed-off-by: 蔡略 <cailue@apache.org>
@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 14, 2025
Copy link
Copy Markdown
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.

Thanks @ClSlaid -- this looks good to me 🙏

awaits for #8951

Why is this waiting on #8951 ?

batch: RecordBatch,
indices: &dyn Array,
) -> Result<(), ArrowError> {
// todo: optimize this to avoid materializing (copying the results of take indices to a new batch)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@ClSlaid
Copy link
Copy Markdown
Contributor Author

ClSlaid commented Dec 14, 2025

Why is this waiting on #8951 ?

Because I dislike resolving git conflicts. :(

Well you just reminded me that I have to resolve it anyway so I'll make this PR a draft and continue to optimize this PR.

@ClSlaid ClSlaid marked this pull request as draft December 14, 2025 14:43
@Dandandan
Copy link
Copy Markdown
Contributor

Why is this waiting on #8951 ?

Because I dislike resolving git conflicts. :(

Well you just reminded me that I have to resolve it anyway so I'll make this PR a draft and continue to optimize this PR.

There doesn't seem to be a conflict? I think we can just merge this PR as is.

@ClSlaid
Copy link
Copy Markdown
Contributor Author

ClSlaid commented Dec 15, 2025

There doesn't seem to be a conflict? I think we can just merge this PR as is.

/cc @alamb

If this PR do completed all optimization, there will be possibility to conflict with the PR mentioned above.

It makes sense to merge this PR first, and I'll further open up a new PR for optimization.

@ClSlaid ClSlaid marked this pull request as ready for review December 15, 2025 15:51
Copy link
Copy Markdown
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.

I think this is a good start -- let's begin with this function and optimize in subsequent PRs

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Dec 15, 2025

Thank you @ClSlaid 🙏 and @Dandandan

@alamb alamb merged commit 91234b5 into apache:main Dec 15, 2025
27 checks passed
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants