Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Jul 23, 2024

Which issue does this PR close?

N/A

Rationale for this change

While working on #11610 I found that there is a wrapper over concat_batches

The concat batches function is now available in arrow-rs so let's use that

What changes are included in this PR?

Use arrow concat_batches directly rather than via a wrapper

Are these changes tested?

By existing CI

Are there any user-facing changes?

No

@alamb
Copy link
Contributor Author

alamb commented Jul 23, 2024

@ozankabak mentioned #11610 (comment) there was work in progress for CoalesceBatches so we might want to make this PR a draft until that work has landed

batches.len(),
row_count
);
arrow::compute::concat_batches(schema, batches)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing function simply wraps concat_batches in arrow and logs some information which I don't think is critical. Note that the row_count is only used for the logging. If reviewers think the trace! is valuable, I will put it back (at the callsites)


// Load all batches and count the rows
let (batches, num_rows, metrics, mut reservation) = stream
let (batches, _num_rows, metrics, mut reservation) = stream
Copy link
Contributor

@Dandandan Dandandan Jul 23, 2024

Choose a reason for hiding this comment

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

The calculation of _num_rows can be removed here now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call -- will do as a follow on PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

3 participants