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

deprecate public export of test_utils #7294

Open
alamb opened this issue Mar 14, 2025 · 4 comments · May be fixed by #7375
Open

deprecate public export of test_utils #7294

alamb opened this issue Mar 14, 2025 · 4 comments · May be fixed by #7375
Labels
arrow Changes to the arrow crate good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Mar 14, 2025

The test_utils package has a non trivial public API (e.g. it exposes rand) even though the code is fairly simple

It would be nice to make this crate more self contained and thus I think we should deprecate and eventually remove test_utils from the public API

          > > 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) 🤔

Originally posted by @alamb in #7128 (comment)

@alamb alamb added good first issue Good for newcomers arrow Changes to the arrow crate labels Mar 14, 2025
@klion26
Copy link
Member

klion26 commented Mar 31, 2025

@alamb I can help to implement this, does this only need to add something like below in arrow/src/util/mode.rs

#[deprecated(
    since = "54.3.2",  <-- the current version is `54.3.1`, so use `54.3.2` here, can be changed if needed
    note = "The `test_util` module is deprecated and will be removed in a future release."
)]
pub mod test_util;

If this is the right way, I will file a pr for it, and if other things need to be done, please give some hints, and I'll adopt them. thanks.

@alamb
Copy link
Contributor Author

alamb commented Mar 31, 2025

@alamb I can help to implement this,

Thanks @klion26

Does this only need to add something like below in arrow/src/util/mode.rs

#[deprecated(
since = "54.3.2", <-- the current version is 54.3.1, so use 54.3.2 here, can be changed if needed
note = "The test_util module is deprecated and will be removed in a future release."
)]
pub mod test_util;
If this is the right way, I will file a pr for it, and if other things need to be done, please give some hints, and I'll adopt them. thanks.

Yes I think this is the right way. The since version should be 55 in this case

@klion26 klion26 linked a pull request Apr 2, 2025 that will close this issue
@rluvaton
Copy link
Contributor

rluvaton commented Apr 3, 2025

I believe tests utils have a lot of value, I would love to have a crate for generating arrays and record batches for easy testing and benchmarking

@alamb
Copy link
Contributor Author

alamb commented Apr 3, 2025

I believe tests utils have a lot of value, I would love to have a crate for generating arrays and record batches for easy testing and benchmarking

Perhaps you can comment on #7375

The reason this got brought up actually was actually some tests I think you may have added in DataFusion that used the test utils. I agree utilities for generating arrays are useful. I think the question is how much benefit we get from having them reused in a library (vs the maintenance cost) compared to, for example, just copying the relevant code where it is needed.

I can see both points of view

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 good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants