Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publically re-export rand with test_utils #7128

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Feb 12, 2025

Which issue does this PR close?

Rationale for this change

As we found out here rand appears in some public APIs and thus it should be available to use with just arrow

For example, in DataFusion we have

https://github.com/apache/datafusion/blob/dfc4ba31ed26fbc84eebb0c39b96ebd8f2c11267/datafusion/functions-aggregate/benches/array_agg.rs#L51-L58

(namely randomly generating a LIstArray using the create_primitive_array function from bench_utils)

Being able to have datafusion's benchmark use the same rand / distribution would let us use both versions

It may just be better to avoid using the arrow bench_utils at all.

What changes are included in this PR?

Propose publically re-exporting rand and using that re-export in the public APIs

Are there any user-facing changes?

New public export when test_utils feature is enabled

@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 12, 2025
@alamb
Copy link
Contributor Author

alamb commented Feb 12, 2025

I don't feel strongly about this and if it is not wanted we can just close this PR

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

I am largely indifferent to this

Copy link
Contributor

@mbrobbel mbrobbel left a comment

Choose a reason for hiding this comment

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

It may just be better to avoid using the arrow bench_utils at all.

+1

But it's also a good idea to merge this.

@alamb
Copy link
Contributor Author

alamb commented Feb 19, 2025

It may just be better to avoid using the arrow bench_utils at all.

+1

But it's also a good idea to merge this.

What do we think about merging this and marking all of bench_utils as deprecated (pointing out that external users should just copy the code into their repo if they want to use them) 🤔

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