Make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs by alamb · Pull Request #7088 · apache/datafusion

alamb · 2023-07-25T14:20:47Z

Which issue does this PR close?

Closes #7087
Closes #7044

Rationale for this change

This implements the prospoal from @jayzhan211 @izveigor @vincev and @smiklos from #7044 (comment)

What changes are included in this PR?

Make unnest consistent with DuckDB/ClickHouse (do not preserve nulls),
add option for preserve_nulls
Add test
update docs

Are these changes tested?

yes

Are there any user-facing changes?

yes, API has changed

alamb · 2023-07-25T14:24:14Z

datafusion/core/tests/dataframe/mod.rs

 }

+#[tokio::test]
+async fn unnest_column_nulls() -> Result<()> {


Here is a test that I think shows the difference between the two modes (or will once we implement it)

alamb · 2023-07-25T14:24:27Z

datafusion/expr/src/logical_plan/plan.rs

+/// Unnest a column that contains a nested list type, replicating
+/// values in the other, non nested rows.
+///
+/// Conceptually this operation is like joining each row with all the


Here is the proposed behavior

alamb · 2023-07-25T14:26:07Z

datafusion/core/src/dataframe.rs

    /// ```
-    pub fn unnest_column(self, column: &str) -> Result<DataFrame> {
+    /// [Unnest]: crate::logical_expr::Unnest
+    pub fn unnest_column(self, column: &str, preserve_nulls: bool) -> Result<DataFrame> {


I think this is different than what @vincev proposed in #7044 (comment)

Specifically, there are three ways to make unnest plans:

unnest function

Dataframe::unnest

LogicalPlanBuilder

It seemed inconsistent to add a preserve_nulls argument in only one place

An alternative would be to add new unest_with_options, DataFrame::unest_with_options and LogicalPlanBuilder::with_options and leave the existing signatures alone.

What do reviewers think?

🤔 seeing the scope of the required changes to add a new parameter, I am thinking _with_options would be preferable

we could just keep the overload but add the options struct instead of only the bool flag.

_with_options looks nice to me too, DataFrame::unnest would call _with_options preserving nulls so that users will keep getting the same behavior as before that is consistent with other data frame libraries and SQL unnest would use the DuckDB/ClickHouse behavior.

Ok, I'll give it a try

smiklos · 2023-07-25T14:51:31Z

Looks great imo, would love to move the improvements form #7002 into a new branch on top of this + implement handling the flag

alamb · 2023-07-26T19:55:13Z

Sorry I did not make progress on this today; I have it on my list for tomorrow (or anyone else feel free to make a PR using any of this PR they want)

smiklos · 2023-07-27T07:25:29Z

Sorry I did not make progress on this today; I have it on my list for tomorrow (or anyone else feel free to make a PR using any of this PR they want)

I can work on this tomorrow if you don't mind.

The plan is to leave (and mark deprecated) the existing UnnestExec and create a new variant that handles options such as preserve_nulls. Then move all api calls to this new physical plan where sql uses perserve_nulls = false, and the dataframe api uses preserve_nulls = true. Right?

alamb · 2023-07-30T11:04:55Z

BTW I am sorry for my delay -- unfortunately sorting out list / nested types is not as high a priority for us at InfluxData at this time, even though I think it is very important for DataFusion overall. Thus I have to work on this project in my spare time.

I am definitely still planning to work on this PR, but if anyone else has time and would like to try and take it on that would also be great 🙏

I was thinking about making UnnestOptions in datafusion_common and then update the API to use it

/// <docs about unnest behavior here>
struct UnestOptions {
  pub preserve_nulls: ...
}

I would suggest adding a new options: UnnestOptions field to the existing Unest LogicalPlan and UnnestExec rather than making entirely new operators

To keep the size of PRs down, I also would personally suggest adding in the API in one PR but implementing the new preserve_nulls: true behavior as a second PR.

alamb · 2023-08-01T13:17:52Z

starting to make another proposal PR...

smiklos · 2023-08-01T13:26:50Z

starting to make another proposal PR...

Sorry for not picking this up. I'm not sure it makes sense to add the Options as a struct vs just a single preserve_nulls field in the logical/physical plan.

Happy to drive the implementation and include the improvements mentioned here

alamb · 2023-08-01T20:52:41Z

Sorry for not picking this up.

No worries -- sorry for the delay

I'm not sure it makes sense to add the Options as a struct vs just a single preserve_nulls field in the logical/physical plan.

Yeah I can see the argument for just a field. The nice thing about the struct was it gave me a place to put the documentation on that could be referenced as well as adding a place that any potential future options could be added. I don't have a strong preference

Happy to drive the implementation and include the improvements mentioned #6961

That would be great

alamb · 2023-08-21T21:16:30Z

This was superceded by #7168

alamb added 5 commits July 20, 2023 14:05

Minor: Add docs and a test for unnest

4c271a2

Merge remote-tracking branch 'apache/main' into alamb/unnest_docs

9fb547e

Update docs

f29f7f6

Begin working on test

2b1d619

Make preserving nulls optional

e81752e

alamb added the api change Changes the API exposed to users of the crate label Jul 25, 2023

github-actions bot added logical-expr Logical plan and expressions core Core DataFusion crate labels Jul 25, 2023

alamb commented Jul 25, 2023

View reviewed changes

alamb mentioned this pull request Jul 25, 2023

Docs and a test for unnest / Proposed consistent API #7044

Closed

smiklos mentioned this pull request Jul 27, 2023

avoid copying listarray in unset exec #7002

Closed

vincev mentioned this pull request Jul 29, 2023

Implement basic unnest function #6796

Closed

7 tasks

alamb mentioned this pull request Jul 30, 2023

make_array null handling with nested version #7136

Closed

alamb mentioned this pull request Aug 1, 2023

Add API to make unnest consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs #7168

Merged

alamb closed this Aug 21, 2023

Conversation

alamb commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

smiklos Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

vincev Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

alamb Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

smiklos commented Jul 25, 2023

Uh oh!

alamb commented Jul 26, 2023

Uh oh!

smiklos commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Jul 30, 2023

Uh oh!

alamb commented Aug 1, 2023

Uh oh!

smiklos commented Aug 1, 2023

Uh oh!

alamb commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Jul 25, 2023 •

edited

Loading

smiklos commented Jul 27, 2023 •

edited

Loading

alamb commented Aug 1, 2023 •

edited

Loading