You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Nagato-Yuzuru (via GitHub)" <gi...@apache.org> on 2026/08/11 01:36:49 UTC

[I] [Epic] Fuzz data generators never produce NULLs [datafusion]

Nagato-Yuzuru opened a new issue, #24243:
URL: https://github.com/apache/datafusion/issues/24243

   ### Context
   
   #11596 and #24208 are both wrong-result bugs in `SortProperties` ordering propagation. Neither one crashes: the optimizer claims an ordering the data doesn't have, and a downstream sort gets elided. Both went unnoticed for a long time, and both need NULLs in the data to be observable at all. 
   
   We already have a [fuzzer](https://github.com/apache/datafusion/blob/main/datafusion/core/tests/fuzz_cases/equivalence/ordering.rs) aimed at exactly this code. The equivalence fuzz tests check `EquivalenceProperties::ordering_satisfy` against ground truth: `is_table_same_after_sort` re-sorts the generated table with arrow's `lexsort_to_indices` (which respects `nulls_first`) and compares row order. That oracle would have caught both bugs. 
   
   The problem is the input side. `generate_table_for_eq_properties` and `generate_table_for_orderings` (fuzz_cases/equivalence/utils.rs) build every column with `Float64Array::from_iter_values`, which takes an iterator of plain f64 (not `Option<f64>`). The generated arrays never contain a null and no null percentage to turn up.
   
   
   ### What I did
   
   I surveyed the rest of the fuzz suite and found the same gap in most of it:
   
   | Fuzzer                                                                          | NULL coverage today                                                        | Oracle                             |
   | ------------------------------------------------------------------------------- | -------------------------------------------------------------------------- | ---------------------------------- |
   | `equivalence/utils.rs` generators (used by ordering/properties/projection fuzz) | none possible (`from_iter_values`)                                         | ground truth (arrow lexsort)       |
   | `sort_preserving_repartition_fuzz.rs` (near-copy of the above generator)        | none possible                                                              | ground truth                       |
   | `limit_fuzz.rs` (TopK)                                                          | columns are `Vec<Option<T>>` but every variant says `// no nulls for now`  | differential (TopK vs full sort)   |
   | `window_fuzz.rs`                                                                | PARTITION BY / ORDER BY columns never null                                 | metamorphic (bounded vs unbounded) |
   | `sort_fuzz.rs`                                                                  | all three staggered generators non-null                                    | reference sort (std sort)   |
   | `merge_fuzz.rs`                                                                 | `(low..high).map(Some)`, always `Some`                                     | differential                       |
   | `join_fuzz.rs`                                                                  | join key columns non-null (a payload filter column already has ~10% nulls) | differential (HJ vs SMJ vs NLJ)    |
   | `aggregate_fuzz.rs` (old `streaming_aggregate_test` path)                       | non-null                                                                   | differential                       |
   
   
   The newer infrastructure already handles this well: `RecordBatchGenerator` picks a null percentage per column from `[0.0, 0.01, 0.1, 0.5]`, and `sort_query_fuzz.rs` even generates `ORDER BY ... NULLS FIRST/LAST` queries. But that path never reaches the `EquivalenceProperties` code where these two bugs live.
   
   ### Describe the solution you'd like
   
   This is a proposal and I'd appreciate feedback on it, especially on scope.
   
   For each fuzzer above:
   
   - If the component under test really does assume non-null input, keep the data  non-null, but say so in the schema (`nullable: false`) or a comment. The absence of  NULLs should be a decision.
   - Otherwise, add NULLs with a randomized percentage. Reuse the existing null-aware  generators (`test-utils/array_gen`, `RecordBatchGenerator`) rather than adding new  one-off ones.
   
   
   ### Task list
   
   This is a rough plan; items may change as earlier ones land and we see what the updated fuzzers turn up
   
   - [ ] Make the `equivalence/utils.rs` generators null-aware, placing NULLs per the  declared orderings. Verify it fails against the pre-fix code for #11596/#24208
   - [ ] Fold `sort_preserving_repartition_fuzz.rs`'s copy of the generator into the fixed one. 
   - [ ] `limit_fuzz.rs`: remove `// no nulls for now`, inject randomized nulls
   - [ ] `window_fuzz.rs`: nulls in PARTITION BY / ORDER BY columns
   - [ ] `sort_fuzz.rs`: nulls in the staggered batch generators
   - [ ] `merge_fuzz.rs`: nulls in the merged streams
   - [ ] `join_fuzz.rs`: nulls in join key columns, to exercise null-equality semantics across the three join implementations
   - [ ] `aggregate_fuzz.rs` old path: add nulls, or migrate the remaining tests to the  null-aware `AggregationFuzzer` framework
   
   Out of scope for now: `pruning.rs` uses a deliberately non-nullable schema, and NULL pruning semantics feel like a separate discussion.
   


-- 
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: github-unsubscribe@datafusion.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [I] [Epic] Fuzz data generators never produce NULLs [datafusion]

Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #24243:
URL: https://github.com/apache/datafusion/issues/24243#issuecomment-5305382669

   This looks good to me


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [I] [Epic] Fuzz data generators never produce NULLs [datafusion]

Posted by "Jithendra2608 (via GitHub)" <gi...@apache.org>.
Jithendra2608 commented on issue #24243:
URL: https://github.com/apache/datafusion/issues/24243#issuecomment-5250708433

   take
   
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [I] [Epic] Fuzz data generators never produce NULLs [datafusion]

Posted by "Nagato-Yuzuru (via GitHub)" <gi...@apache.org>.
Nagato-Yuzuru commented on issue #24243:
URL: https://github.com/apache/datafusion/issues/24243#issuecomment-6019392319

   @jayzhan211 Thanks for the pointer. I'd prefer to keep this in-repo though, mainly because:
   
   - The in-repo fuzz runs in the merge queue, so it can block a regression before it merges
   - The fuzz already exists here; the gap is only in its generators without NULLs
   
   I have a working change #26086 : with it, reverting #24206 and #24276 makes the fuzz fail, and it passes on main.  
   
   I plan to send it as separate PRs if one of them surfaces a new bug it can land together with its fix without blocking the others.
   
   Happy to reconsider if you see it differently. 


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [I] [Epic] Fuzz data generators never produce NULLs [datafusion]

Posted by "Nagato-Yuzuru (via GitHub)" <gi...@apache.org>.
Nagato-Yuzuru commented on issue #24243:
URL: https://github.com/apache/datafusion/issues/24243#issuecomment-5250767920

   @Jithendra2608 I think it would be best for us to discuss this and get confirmation from the maintainers.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org