You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/08/01 13:57:24 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
alamb opened a new pull request, #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168
# Which issue does this PR close?
re https://github.com/apache/arrow-datafusion/issues/7087
closes https://github.com/apache/arrow-datafusion/pull/7088
closes https://github.com/apache/arrow-datafusion/pull/7044
# Rationale for this change
The unnest functionality is both under documented in DataFusion as well as inconsistent with respect to null handling with DuckDB and ClickHouse.
# What changes are included in this PR?
1. Document what `unnest` does, with an example
2. Add `UnnestOptions` that controls the behavior of `unnest` with null inputs and `unest_with_options` and related functions
3. Add tests showing how to use `UnnestOptions` and
note this PR *DOES NOT* actually change the `unnest` behavior -- I believe that should be done as a separate PR
# Are these changes tested?
Yes
# Are there any user-facing changes?
New API (but existing APIs stay the same)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#discussion_r1280721620
##########
datafusion/core/src/dataframe.rs:
##########
@@ -151,20 +151,38 @@ impl DataFrame {
/// Expand each list element of a column to multiple rows.
///
+ /// Seee also:
+ ///
+ /// 1. [`UnnestOptions`] documentation for the behavior of `unnest`
+ /// 2. [`Self::unnest_column_with_options`]
+ ///
/// ```
/// # use datafusion::prelude::*;
/// # use datafusion::error::Result;
/// # #[tokio::main]
/// # async fn main() -> Result<()> {
/// let ctx = SessionContext::new();
/// let df = ctx.read_csv("tests/data/example.csv", CsvReadOptions::new()).await?;
- /// let df = df.unnest_column("a")?;
+ /// let df = df.unnest_column_wtith_optons("a")?;
Review Comment:
```suggestion
/// let df = df.unnest_column_with_options("a")?;
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#issuecomment-1671977161
I plan to merge this PR tomorrow unless i hear otherwise
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#issuecomment-1664089858
> https://github.com/apache/arrow-datafusion/pull/7088 should not be marked as #closes just yet.
Thanks, removed
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#discussion_r1280690763
##########
datafusion/core/src/dataframe.rs:
##########
@@ -151,20 +151,38 @@ impl DataFrame {
/// Expand each list element of a column to multiple rows.
///
+ /// Seee also:
+ ///
+ /// 1. [`UnnestOptions`] documentation for the behavior of `unnest`
+ /// 2. [`Self::unnest_column_with_options`]
+ ///
/// ```
/// # use datafusion::prelude::*;
/// # use datafusion::error::Result;
/// # #[tokio::main]
/// # async fn main() -> Result<()> {
/// let ctx = SessionContext::new();
/// let df = ctx.read_csv("tests/data/example.csv", CsvReadOptions::new()).await?;
- /// let df = df.unnest_column("a")?;
+ /// let df = df.unnest_column_wtith_optons("a")?;
/// # Ok(())
/// # }
/// ```
pub fn unnest_column(self, column: &str) -> Result<DataFrame> {
+ self.unnest_column_with_options(column, UnnestOptions::new())
+ }
+
+ /// Expand each list element of a column to multiple rows, with
+ /// behavior controlled by [`UnnestOptions`].
+ ///
+ /// Please see the documentation on [`UnnestOptions`] for more
+ /// details about the meaning of unnest.
+ pub fn unnest_column_with_options(
Review Comment:
Here is the key -- add a `_with_options` variant and link to documentation
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -1044,6 +1044,82 @@ async fn unnest_columns() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn unnest_column_preserve_nulls_not_supported() -> Result<()> {
+ // Unnest, preserving nulls not yet supported
+ let options = UnnestOptions::new().with_preserve_nulls(true);
+
+ let results = table_with_lists_and_nulls()
+ .await?
+ .clone()
+ .unnest_column_with_options("list", options)?
+ .collect()
+ .await;
+
+ assert_eq!(
+ results.unwrap_err().to_string(),
+ "This feature is not implemented: Unest with preserve_nulls=true"
Review Comment:
preserve_nulls is not yet supported, but here is a space for it
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] smiklos commented on a diff in pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "smiklos (via GitHub)" <gi...@apache.org>.
smiklos commented on code in PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#discussion_r1280705422
##########
datafusion/core/src/physical_plan/unnest.rs:
##########
@@ -133,6 +146,12 @@ impl ExecutionPlan for UnnestExec {
) -> Result<SendableRecordBatchStream> {
let input = self.input.execute(partition, context)?;
+ if self.options.preserve_nulls {
Review Comment:
Preserve_nulls is the default behavior, shouldn't we blow up in case someone sets it to false?
##########
datafusion/core/src/dataframe.rs:
##########
@@ -151,20 +151,38 @@ impl DataFrame {
/// Expand each list element of a column to multiple rows.
///
+ /// Seee also:
+ ///
+ /// 1. [`UnnestOptions`] documentation for the behavior of `unnest`
+ /// 2. [`Self::unnest_column_with_options`]
+ ///
/// ```
/// # use datafusion::prelude::*;
/// # use datafusion::error::Result;
/// # #[tokio::main]
/// # async fn main() -> Result<()> {
/// let ctx = SessionContext::new();
/// let df = ctx.read_csv("tests/data/example.csv", CsvReadOptions::new()).await?;
- /// let df = df.unnest_column("a")?;
+ /// let df = df.unnest_column_wtith_optons("a")?;
Review Comment:
typo
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#discussion_r1280722125
##########
datafusion/core/src/physical_plan/unnest.rs:
##########
@@ -133,6 +146,12 @@ impl ExecutionPlan for UnnestExec {
) -> Result<SendableRecordBatchStream> {
let input = self.input.execute(partition, context)?;
+ if self.options.preserve_nulls {
Review Comment:
I got this backwards -- I will fix
##########
datafusion/core/src/physical_plan/unnest.rs:
##########
@@ -133,6 +146,12 @@ impl ExecutionPlan for UnnestExec {
) -> Result<SendableRecordBatchStream> {
let input = self.input.execute(partition, context)?;
+ if self.options.preserve_nulls {
Review Comment:
I got this backwards -- I will fix. Thank you.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#discussion_r1280727375
##########
datafusion/core/src/physical_plan/unnest.rs:
##########
@@ -133,6 +146,12 @@ impl ExecutionPlan for UnnestExec {
) -> Result<SendableRecordBatchStream> {
let input = self.input.execute(partition, context)?;
+ if self.options.preserve_nulls {
Review Comment:
Fixed in 5b975d256
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#issuecomment-1673214055
Thanks again everyone for the reviews -- I can't wait to see what we build with this
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7168: Add API to make `unnest` consistent with DuckDB/ClickHouse, add option for preserve_nulls, update docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7168:
URL: https://github.com/apache/arrow-datafusion/pull/7168#issuecomment-1669696847
@jayzhan211 and @izveigor -- what do you think about merging this PR / improved API for unnest?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org