Skip to content

refactor: remove uses of arrow_buffer & arrow_array and use reexport in arrow instead - #14503

Merged
alamb merged 11 commits into
apache:mainfrom
Chen-Yuan-Lai:use_reexport_in_arrow
Feb 5, 2025
Merged

refactor: remove uses of arrow_buffer & arrow_array and use reexport in arrow instead#14503
alamb merged 11 commits into
apache:mainfrom
Chen-Yuan-Lai:use_reexport_in_arrow

Conversation

@Chen-Yuan-Lai

@Chen-Yuan-LaiChen-Yuan-Lai commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#14115.

Rationale for this change

As #14115 (comment) says, we should use reexport in arrow as much as possible.

What changes are included in this PR?

Are these changes tested?

  1. use arrow_buffer:: xxx -> use arrow::buffer::xxx
  2. use arrow_array:: xxx -> use arrow::array::xxx

I leave some imports as it is when those modules weren't reexported in arrow

Are there any user-facing changes?

@github-actionsgithub-actionsBot added physical-expr Changes to the physical-expr crates core Core DataFusion crate common Related to common crate functions Changes to functions implementation labels Feb 5, 2025
@Chen-Yuan-LaiChen-Yuan-Lai mentioned this pull request Feb 5, 2025
Closed
alamb
alamb approved these changes Feb 5, 2025
View reviewed changes

@alambalamb left a comment

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.

Thank you so much @Chen-Yuan-Lai

I also took the liberty of removing a few more uses of arrow_buffer and removed it from the Cargo.toml files and pushed that change to this PR

It turns out OffsetBufferBuilder is still used in some places so I made an upstream PR in arrow-rs to help:

Any chance you are willing to do the same thing for arrow-array?

@github-actionsgithub-actionsBot added the substrait Changes to the substrait crate label Feb 5, 2025
alamb
alamb reviewed Feb 5, 2025
View reviewed changes
Comment threaddatafusion/common/Cargo.toml
], optional = true }
arrow = { workspace = true }
arrow-array = { workspace = true }
arrow-buffer = { workspace = true }

@alambalambFeb 5, 2025

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.

I also removed the dependency directly from Cargo.toml

@Chen-Yuan-Lai

Chen-Yuan-Lai commented Feb 5, 2025

Copy link
Copy Markdown
ContributorAuthor

Any chance you are willing to do the same thing for arrow-array?

Sure : )

@alamb

alamb commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

Any chance you are willing to do the same thing for arrow-array?

To be clear, I suggest we do this in a separate PR

@alamb

alamb commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

I plan to merge this PR when the CI checks pass

@alamb
alamb merged commit d5ff3e7 into apache:mainFeb 5, 2025
@alamb

alamb commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

Thanks again @Chen-Yuan-Lai

@Chen-Yuan-LaiChen-Yuan-Lai mentioned this pull request Feb 6, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes

Assignees

No one assigned

Labels

commonRelated to common cratecoreCore DataFusion cratefunctionsChanges to functions implementationphysical-exprChanges to the physical-expr cratessubstraitChanges to the substrait crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use NullBufferBuilder instead of BooleanBufferBuilder for creating Null masks

2 participants

@Chen-Yuan-Lai@alamb