feat: Add support for unnest_outer function for arrays. - #22100
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
comphead
left a comment
There was a problem hiding this comment.
Thanks @athlcode explode_outer is not like other builtin function so it has to be in the core.
Please add tests, more tests the better, including mix with nulls, nested combinations with explode and explode_outer and supported datatypes
thank you, added more test cases |
|
Thanks @athlcode my bad, I just realized I haven't clarified tests needs to be added to |
Thanks @comphead for the clarification. None of the test cases are currently reproducible via SLT because there's no SQL surface for NullHandling::PreserveAndExpandEmpty. To enable SLT coverage we need to add SQL recognition of explode_outer(col) (and probably explode(col) for symmetry) and map them to UNNEST with the right NullHandling variant. |
|
we would still need to have |
Added slt tests at datafusion/sqllogictest/test_files/spark/generator/explode_outer.slt, exercises explode_outer via the Spark SQL dialect path |
Thanks @athlcode sorry I really missed this comments, it makes sense to me, lets remove Spark from comments in the core, as this would be confusing, and other than that, the PR would be good to go |
|
@comphead thanks, I have removed spark from the comments, |
explode_outer function for arrays
|
Thanks @athlcode. I checked some of engines for Lets move tests from |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22100 +/- ##
==========================================
- Coverage 80.69% 80.69% -0.01%
==========================================
Files 1095 1095
Lines 372626 373108 +482
Branches 372626 373108 +482
==========================================
+ Hits 300700 301061 +361
- Misses 53978 54095 +117
- Partials 17948 17952 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
thanks @comphead. moved tests into |
explode_outer function for arraysunnest_outer function for arrays.
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#19053. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> DataFusion's 'unnest' had no way to express Spark 'explode_outer' semantics, empty input lists were silently dropped, even with 'preserve_nulls' = true. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Adds a third unnest behavior that produces a `NULL` row for empty input lists, by replacing `UnnestOptions.preserve_nulls: bool` with a `NullHandling { Drop, Preserve, PreserveAndExpandEmpty }` enum. The `with_preserve_nulls(bool)` builder is kept as a backward-compat shim. ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes, new unit test for the empty-list case, extended longest-length and DataFrame `unnest_column_nulls` tests, and existing proto round-trip coverage. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> The `preserve_nulls` field on `UnnestOptions` is renamed to `null_handling`. The `with_preserve_nulls(bool)` builder is preserved, so most callers are unaffected. Add the `api change` label for the field rename.
- Use upstream's public `ListUnnest` instead of the copy in the vendored region. It is the one item there that is not private to `datafusion-physical-plan`, so copying it was unjustified and left Comet exporting a same-named twin of a public DataFusion type. - Correct the deletion trigger. apache#5210 tracks adopting upstream `unnest_outer` (apache/datafusion#22100), not batch size, so closing it is not a signal to delete this fork; the trigger is apache/datafusion#24384. - Stop claiming the vendored region is byte-identical. Comet's rustfmt reflows it (`max_width = 100` vs upstream's 90), so document the audit recipe instead. - Drop the comment claiming everything below it was Comet code, which contradicted the banner further down the same file. - Fix a test that did not test what it was named for: with 10-element arrays at `batch_size = 8` every row overflows, so `respects_batch_size` only re-covered the oversized-build path. Use 3-element arrays so chunks pack several input rows, and assert the exact `[6, 6, 6, 6, 6]` shape. - Collapse the two near-identical null-handling tests onto a shared helper, add `sizes`/`seq` test helpers, and use the `AsArray` idiom already in scope. - Simplify: `Option::filter` over a match with a wildcard arm, `Count` imported rather than fully qualified twice, and `elapsed_compute().timer()` without the needless clone, matching scan.rs and shuffle_scan.rs.
Which issue does this PR close?
explode_outerfunction for arrays #19053.Rationale for this change
DataFusion's 'unnest' had no way to express Spark 'explode_outer' semantics, empty input lists were silently dropped, even with 'preserve_nulls' = true.
What changes are included in this PR?
Adds a third unnest behavior that produces a
NULLrow for empty input lists, by replacingUnnestOptions.preserve_nulls: boolwith aNullHandling { Drop, Preserve, PreserveAndExpandEmpty }enum. Thewith_preserve_nulls(bool)builder is kept as a backward-compat shim.Are these changes tested?
Yes, new unit test for the empty-list case, extended longest-length and DataFrame
unnest_column_nullstests, and existing proto round-trip coverage.Are there any user-facing changes?
The
preserve_nullsfield onUnnestOptionsis renamed tonull_handling. Thewith_preserve_nulls(bool)builder is preserved, so most callers are unaffected. Add theapi changelabel for the field rename.