jayzhan211 commented on code in PR #27258:
URL: https://github.com/apache/datafusion/pull/27258#discussion_r4539031430
##########
datafusion/physical-plan/src/joins/key_range_bitmap.rs:
##########
@@ -107,7 +107,7 @@ impl KeyRangeBitmap {
/// Coarser than the parts' own bitmaps where its buckets are wider, but
/// never clears a bucket a part may hold a key in. `None` when the key
/// type has no `u64` ordering, or when nothing can be excluded.
- pub(crate) fn union(
+ pub fn union(
Review Comment:
Same for `union`:
```suggestion
/// type has no `u64` ordering, or when nothing can be excluded.
///
/// # Public Only for Internal Use:
/// The `datafusion-fuzz` tests require this, but it is not part of the
/// supported public API.
#[doc(hidden)]
pub fn union(
```
##########
datafusion/physical-plan/src/joins/key_range_bitmap.rs:
##########
@@ -16,7 +16,7 @@ use crate::joins::array_map::ArrayMap;
const MAX_BUCKETS: u64 = 1 << 20;
/// One build partition's contribution to [`KeyRangeBitmap::union`].
-pub(crate) enum KeyRangePart<'a> {
+pub enum KeyRangePart<'a> {
Review Comment:
This makes `KeyRangePart`, `union` and the four new getters supported public
API of `datafusion-physical-plan`, only so the fuzz crate can call them.
[api-health.md](https://datafusion.apache.org/contributor-guide/api-health.html#internal-public-apis)
says items shared across crates for tests should be `#[doc(hidden)]` with a
"Public Only for Internal Use" note, like `HashTableLookupExpr::new` does for
the `datafusion-proto` tests. Otherwise, changing the bucket layout later
becomes a breaking change. I applied the three suggestions on this file
locally: rustdoc no longer lists these items, and `cargo doc` with `-D
warnings`, clippy and the fuzz run all pass.
```suggestion
/// One build partition's contribution to [`KeyRangeBitmap::union`].
///
/// # Public Only for Internal Use:
/// The `datafusion-fuzz` tests require this, but it is not part of the
/// supported public API.
#[doc(hidden)]
pub enum KeyRangePart<'a> {
```
##########
datafusion/fuzz/tests/fuzz.rs:
##########
@@ -20,15 +20,27 @@ mod suites;
#[tokio::test(flavor = "multi_thread")]
async fn fuzz_tests() -> Result<()> {
let spill_pool = suites::spill_pool::SpillPoolFuzzer;
- let suites = [Suite::new(
- &spill_pool,
- RunConfig {
- run_seed: Some(0),
- max_cases: Some(1000),
- time_limit: None,
- cases_per_dataset: 1000,
- },
- )];
+ let key_range_bitmap = suites::key_range_bitmap::KeyRangeBitmapFuzzer;
+ let suites = [
+ Suite::new(
+ &spill_pool,
+ RunConfig {
+ run_seed: Some(0),
+ max_cases: Some(1000),
+ time_limit: None,
+ cases_per_dataset: 1000,
+ },
+ ),
+ Suite::new(
+ &key_range_bitmap,
+ RunConfig {
+ run_seed: Some(0),
+ max_cases: Some(256),
+ time_limit: None,
+ cases_per_dataset: 256,
Review Comment:
Before this PR, `union_fuzz` ran 10,000 seeds in the merge queue's
`fuzz-legacy` step (`DATAFUSION_KEY_RANGE_UNION_FUZZ_ITERATIONS=10000`). Now it
runs 256, about 39x fewer. The spill-pool migration in #27048 kept its old CI
count (1000), and 10,000 cases of this suite take 3.9 s locally in the `ci`
profile, so could we keep 10,000? With run seed 0 that runs the same seeds
0..9999 as before.
```suggestion
max_cases: Some(10000),
time_limit: None,
cases_per_dataset: 10000,
```
##########
datafusion/physical-plan/src/joins/key_range_bitmap.rs:
##########
@@ -250,6 +250,26 @@ impl KeyRangeBitmap {
pub fn fill_stats(&self) -> (usize, usize) {
(self.bits.count_set_bits(), self.bits.len())
}
+
+ /// Smallest key, in the `u64` ordering the join's array map uses.
Review Comment:
And for the four getters, a separate hidden `impl` block keeps them out of
the docs with one attribute:
```suggestion
}
}
/// # Public Only for Internal Use:
/// The `datafusion-fuzz` tests require these accessors, but they are not
part
/// of the supported public API.
#[doc(hidden)]
impl KeyRangeBitmap {
/// Smallest key, in the `u64` ordering the join's array map uses.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
