You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rgehan (via GitHub)" <gi...@apache.org> on 2025/10/29 12:01:30 UTC
[PR] Reproducer tests for #9898 [datafusion]
rgehan opened a new pull request, #18352:
URL: https://github.com/apache/datafusion/pull/18352
## Which issue does this PR close?
None, but relates to issue #9898
## Rationale for this change
N/A
## What changes are included in this PR?
This PR adds reproducer tests demonstrating issues with suboptimal optimizations performed on plans that mix pre-sorted parquets and `SortExec` under an UNION.
Two sets of tests included:
- Unit tests in `datafusion/core/tests/physical_optimizer/enforce_sorting.rs`
- E2E-ish tests in `datafusion/core/tests/dataframe/mod.rs`, starting from logical plans simulating our use-case
## Are these changes tested?
N/A
## Are there any user-facing changes?
N/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@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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #18352:
URL: https://github.com/apache/datafusion/pull/18352
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3467421434
> @NGA-TRAN thanks a lot for the review and clarifications! I've adapted the PR to hopefully make it clearer what's expected / what's not. I'll also create an issue
Once you've updated the PR so that three tests pass and one fails, mark the failing test as ignored (with comment) and move it to "ready for review." It's great that the repro is included in the repo.
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3468623628
@NGA-TRAN I've applied the requested changes 👍
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2474561646
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
Review Comment:
👍 This is the key here: the setting that forces DF to execute sorts in a per-partition fashion and merge afterwards
https://github.com/apache/datafusion/blob/e432d5561e5ef16348a3d0ab7cfb553795e31683/datafusion/common/src/config.rs#L919
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
Review Comment:
Do you know whether the file is actually sorted or you just add this function to trick the planner to plan this file as it is sorted?
##########
datafusion/core/tests/physical_optimizer/enforce_sorting.rs:
##########
@@ -359,6 +359,79 @@ async fn test_union_inputs_different_sorted2() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_with_repartition_sorts_true() -> Result<()> {
+ reproducer_impl(true).await?; // ✅ Passes
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_with_repartition_sorts_false() -> Result<()> {
+ reproducer_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // OutputRequirementExec: order_by=[(nullable_col@0, asc)], dist_by=SinglePartition
+ // SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0 ASC], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_impl(repartition_sorts: bool) -> Result<()> {
Review Comment:
Similarly, can you have this function to accept 2 inputs and have all its 2 tests passed
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
Review Comment:
Can you try to replace this with:
`let union = source_unsorted_resorted.union(source_sorted)?;`
to see if it works correctly?
##########
datafusion/core/tests/physical_optimizer/enforce_sorting.rs:
##########
@@ -359,6 +359,79 @@ async fn test_union_inputs_different_sorted2() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_with_repartition_sorts_true() -> Result<()> {
+ reproducer_impl(true).await?; // ✅ Passes
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_with_repartition_sorts_false() -> Result<()> {
+ reproducer_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
Review Comment:
It is expected that this test fails here. See https://github.com/apache/datafusion/blob/e432d5561e5ef16348a3d0ab7cfb553795e31683/datafusion/common/src/config.rs#L919
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
+
+ // To be able to remove user specific paths from the plan, for stable assertions
+ let testdata_clean = Path::new(&testdata).canonicalize()?.display().to_string();
+ let testdata_clean = testdata_clean.strip_prefix("/").unwrap_or(&testdata_clean);
+
+ let plan = df.explain(false, false)?.collect().await?;
+ assert_snapshot!(
+ pretty_format_batches(&plan)?.to_string().replace(&testdata_clean, "{testdata}"),
+ @r#"
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortPreservingMergeExec: [id@0 ASC NULLS LAST] |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
Review Comment:
👍 Since the 2 input streams out of the union are sorted, the 2 streams coming out from Partial Aggregate are also sorted. Thus, we should only do the merge
And this only happens with `repartition_sorts` = true
https://github.com/apache/datafusion/blob/e432d5561e5ef16348a3d0ab7cfb553795e31683/datafusion/common/src/config.rs#L919
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
Review Comment:
This is unexpected and the reason of your reproducer 👍
See my comment for `reproducer_e2e_impl` below to modify the test a bit to make it easier for reviewers understand
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
Review Comment:
Can you modify this function to accept 2 parameters: `repartition_sorts` and `expected_plan`? Then at the comparison step, you compare with `expected_plan`. This will help make the tests clearer . You have 4 tests in this PR and only one should fail. The other 3 will pass
##########
datafusion/core/tests/physical_optimizer/enforce_sorting.rs:
##########
@@ -359,6 +359,79 @@ async fn test_union_inputs_different_sorted2() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_with_repartition_sorts_true() -> Result<()> {
+ reproducer_impl(true).await?; // ✅ Passes
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_with_repartition_sorts_false() -> Result<()> {
+ reproducer_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // OutputRequirementExec: order_by=[(nullable_col@0, asc)], dist_by=SinglePartition
+ // SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0 ASC], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_impl(repartition_sorts: bool) -> Result<()> {
+ let schema = create_test_schema()?;
+
+ // Source 1, will be sorted explicitly (on `nullable_col`)
+ let source1 = parquet_exec(schema.clone());
+ let ordering1 = [sort_expr("nullable_col", &schema)].into();
+ let sort1 = sort_exec(ordering1, source1.clone());
+
+ // Source 2, pre-sorted (on `nullable_col`)
+ let parquet_ordering: LexOrdering = [sort_expr("nullable_col", &schema)].into();
+ let source2 = parquet_exec_with_sort(schema.clone(), vec![parquet_ordering.clone()]);
+
+ let union = union_exec(vec![sort1, source2]);
+
+ let coalesced = coalesce_partitions_exec(union);
+
+ // Required sorted / single partitioned output
+ let requirement = [PhysicalSortRequirement::new(
+ col("nullable_col", &schema)?,
+ Some(SortOptions::new(false, true)),
+ )]
+ .into();
+ let physical_plan = Arc::new(OutputRequirementExec::new(
+ coalesced,
+ Some(OrderingRequirements::new(requirement)),
+ Distribution::SinglePartition,
+ None,
+ ));
+
+ let test = EnforceSortingTest::new(physical_plan).with_repartition_sorts(repartition_sorts);
+
+ assert_snapshot!(test.run(), @r"
+ Input Plan:
+ OutputRequirementExec: order_by=[(nullable_col@0, asc)], dist_by=SinglePartition
+ CoalescePartitionsExec
+ UnionExec
+ SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0 ASC], file_type=parquet
+
+ Optimized Plan:
+ OutputRequirementExec: order_by=[(nullable_col@0, asc)], dist_by=SinglePartition
+ SortPreservingMergeExec: [nullable_col@0 ASC]
+ UnionExec
+ SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0 ASC], file_type=parquet
+ ");
Review Comment:
So this test is to let us know we need [repartition_sorts](https://github.com/apache/datafusion/blob/e432d5561e5ef16348a3d0ab7cfb553795e31683/datafusion/common/src/config.rs#L919) = true for it to work. This works as expected: DF understands the 2 input of the Union is sorted and only do the merge after that. It will fail if repartition_sorts = false
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
Review Comment:
Now let us verify this plan without optimization first to see it is the plan. Can you add this?
Note: you may hit compile error because I have not run it
```
let input_plan = displayable(df.as_ref()).indent(true).to_string();
assert_snapshot!( input_plan, @r#"your_expected_input_plan");
```
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
Review Comment:
Since `repartition_sorts`= false. This test fails as expected. I would modify the test to make it pass instead of not pass. See my comment for `reproducer_e2e_impl` below
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2476859655
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
Review Comment:
The file is not actually sorted no, but I was hoping this was a valid way of making the planner think it is, and plan accordingly.
Can this cause issues?
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2478523026
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
Review Comment:
The plan is not commented out.
I asked @rgehan to add the relevant part of explain verbose for us see when the issue happens that was the commented out plan
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3467034193
@NGA-TRAN thanks a lot for the review and clarifications! I've adapted the PR to hopefully make it clearer what's expected / what's not. I'll also create an issue
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477152877
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
Review Comment:
I haven't managed to manually generate a unoptimized physical plan from the dataframe itself, as the `create_initial_plan` method of the planner is private.
The only solution I'm aware of would be to use a verbose explain (below), which would log everything.
--
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: [PR] Reproducer tests for #18380 [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2478495955
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
Review Comment:
why are we ignoring the test when the plan is also commented out in the body?
--
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: [PR] Reproducer tests for #18380 [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2478407869
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
Review Comment:
```suggestion
// Test with `repartition_sorts` disabled, causing a full resort of the data
async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
```
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
Review Comment:
```suggestion
// Test with `repartition_sorts` enabled to preserve pre-sorted partitions and avoid resorting
async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
```
##########
datafusion/core/tests/physical_optimizer/enforce_sorting.rs:
##########
@@ -359,6 +359,87 @@ async fn test_union_inputs_different_sorted2() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
Review Comment:
```suggestion
// Test with `repartition_sorts` enabled to preserve pre-sorted partitions and avoid resorting
async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
```
##########
datafusion/core/tests/physical_optimizer/enforce_sorting.rs:
##########
@@ -359,6 +359,87 @@ async fn test_union_inputs_different_sorted2() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(true).await?,
+ @r"
+ Input Plan:
+ OutputRequirementExec: order_by=[(nullable_col@0, asc)], dist_by=SinglePartition
+ CoalescePartitionsExec
+ UnionExec
+ SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0 ASC], file_type=parquet
+
+ Optimized Plan:
+ OutputRequirementExec: order_by=[(nullable_col@0, asc)], dist_by=SinglePartition
+ SortPreservingMergeExec: [nullable_col@0 ASC]
+ UnionExec
+ SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
+ DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], output_ordering=[nullable_col@0 ASC], file_type=parquet
+ ");
+ Ok(())
+}
+
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
Review Comment:
```suggestion
// Test with `repartition_sorts` disabled, causing a full resort of the data
async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
```
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(true).await?,
+ @r#"
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortPreservingMergeExec: [id@0 ASC NULLS LAST] |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ //
+ // === Excerpt from the verbose explain ===
+ //
+ // Physical_plan after EnforceDistribution:
+ //
+ // OutputRequirementExec: order_by=[], dist_by=Unspecified
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ // Physical_plan after EnforceSorting:
Review Comment:
You might want to keep the exact output, including the no-op optimizer step in between—it helps others see exactly what’s happening.
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2478524356
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(true).await?,
+ @r#"
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortPreservingMergeExec: [id@0 ASC NULLS LAST] |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ //
+ // === Excerpt from the verbose explain ===
+ //
+ // Physical_plan after EnforceDistribution:
+ //
+ // OutputRequirementExec: order_by=[], dist_by=Unspecified
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ // Physical_plan after EnforceSorting:
Review Comment:
I didn't want to spam too much, but sure I can add the full explain 👍
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2478746309
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(true).await?,
+ @r#"
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortPreservingMergeExec: [id@0 ASC NULLS LAST] |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ //
+ // === Excerpt from the verbose explain ===
+ //
+ // Physical_plan after EnforceDistribution:
+ //
+ // OutputRequirementExec: order_by=[], dist_by=Unspecified
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ // Physical_plan after EnforceSorting:
Review Comment:
No problem, I've reverted to the excert, but with the intermediate optimizer step 👍 (it's `CombinePartialFinalAggregate`)
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477091930
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
Review Comment:
It doesn't appear to change the output plan
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477176908
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
Review Comment:
In the meantime, I've generated a verbose explain for the failing test: [see gist](https://gist.github.com/rgehan/b632f5e106d60240876fa15df0d007e4)
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477450389
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
Review Comment:
> Can this cause issues?
Likely not but I am not % sure if we do anything special with parquet file.
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
Review Comment:
The verbose explain is super helpful. The issue is at `physical_plan after EnforceSorting`. When you create an issue, copy both explain and explain verbose in your description. In the code reproducer (this PR), can you copy from line 100 to line 120 of your verbose in comments to show the issue?
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477578257
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
Review Comment:
You can copy the above suggestion with a bit description in your ticket https://github.com/apache/datafusion/issues/18380
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477450389
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
Review Comment:
> Can this cause issues?
Likely not but I am not % sure if we do anything special with parquet file.
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477159486
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
Review Comment:
I've adapted all tests, this one is the only remaining one that fails, as expected 👍
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "NGA-TRAN (via GitHub)" <gi...@apache.org>.
NGA-TRAN commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2478646456
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(true).await?,
+ @r#"
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortPreservingMergeExec: [id@0 ASC NULLS LAST] |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ //
+ // === Excerpt from the verbose explain ===
+ //
+ // Physical_plan after EnforceDistribution:
+ //
+ // OutputRequirementExec: order_by=[], dist_by=Unspecified
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ // Physical_plan after EnforceSorting:
Review Comment:
Sorry. I did ot mean the full verbose 🙂
I remember there was a step in between that does nothing (one line). If I misread, please ignore
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,152 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_false() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
+ @r#"
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | CoalescePartitionsExec |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[] |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+ Ok(())
+}
+
+#[ignore] // See https://github.com/apache/datafusion/issues/18380
+#[tokio::test]
+async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_repartition_sorts_true() -> Result<()> {
+ assert_snapshot!(
+ union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(true).await?,
+ @r#"
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | plan_type | plan |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ | logical_plan | Aggregate: groupBy=[[id]], aggr=[[]] |
+ | | Union |
+ | | TableScan: sorted projection=[id] |
+ | | Sort: unsorted.id ASC NULLS LAST |
+ | | TableScan: unsorted projection=[id] |
+ | physical_plan | AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | SortPreservingMergeExec: [id@0 ASC NULLS LAST] |
+ | | AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted |
+ | | UnionExec |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet |
+ | | SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false] |
+ | | DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet |
+ | | |
+ +---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
+ "#);
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ //
+ // === Excerpt from the verbose explain ===
+ //
+ // Physical_plan after EnforceDistribution:
+ //
+ // OutputRequirementExec: order_by=[], dist_by=Unspecified
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+ //
+ // Physical_plan after EnforceSorting:
Review Comment:
Sorry. I did not mean the full verbose 🙂
I remember there was a step in between that does nothing (one line). If I misread, please ignore
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3467360787
Here's the corresponding feature request: https://github.com/apache/datafusion/issues/18380
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3476294135
I tweaked the tests to use simpler snapshots (no more table format), hopefully this should pass on the CI now 👍
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2477843583
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
+ )
+ .await?;
+
+ // Register "unsorted" table
+ ctx.register_parquet(
+ "unsorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ )
+ .await?;
+
+ let source_sorted = ctx
+ .table("sorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted = ctx
+ .table("unsorted")
+ .await
+ .unwrap()
+ .select(vec![col("id")])
+ .unwrap();
+
+ let source_unsorted_resorted = source_unsorted
+ .sort(vec![col("id").sort(true, false)])?;
+
+ let union = source_sorted.union(source_unsorted_resorted)?;
+
+ let agg = union.aggregate(vec![col("id")], vec![])?;
+
+ let df = agg;
Review Comment:
Done, I've added an excerpt from the verbose explain to the comment, and also to the issue 👍
--
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: [PR] Reproducer tests for #9898 [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on code in PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#discussion_r2472752391
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -2996,6 +2997,119 @@ async fn test_count_wildcard_on_window() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_false() -> Result<()> {
+ reproducer_e2e_impl(false).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]
+ // CoalescePartitionsExec
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+#[tokio::test]
+async fn reproducer_e2e_with_repartition_sorts_true() -> Result<()> {
+ reproducer_e2e_impl(true).await?;
+
+ // 💥 Doesn't pass, and generates this plan:
+ //
+ // AggregateExec: mode=Final, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+ // SortPreservingMergeExec: [id@0 ASC NULLS LAST]
+ // SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[true]
+ // AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[]
+ // UnionExec
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+ // DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], file_type=parquet
+
+ Ok(())
+}
+
+async fn reproducer_e2e_impl(repartition_sorts: bool) -> Result<()> {
+ let config = SessionConfig::default()
+ .with_target_partitions(1)
+ .with_repartition_sorts(repartition_sorts);
+ let ctx = SessionContext::new_with_config(config);
+
+ let testdata = parquet_test_data();
+
+ // Register "sorted" table, that is sorted
+ ctx.register_parquet(
+ "sorted",
+ &format!("{testdata}/alltypes_tiny_pages.parquet"),
+ ParquetReadOptions::default()
+ .file_sort_order(vec![vec![col("id").sort(true, false)]]),
Review Comment:
(Sidenote: Interestingly, with `nulls_first: true` (L3074 too), even with the fixes from #9867, the plan includes an extra `SortExec` node that re-sorts with nulls last. I'm not sure whether that's on purpose, or if there's another issue)
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "rgehan (via GitHub)" <gi...@apache.org>.
rgehan commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3476268472
I fixed the formatting/clippy issues, but [one of the tests](https://github.com/apache/datafusion/actions/runs/18947670299/job/54225759826?pr=18352) is still failing for formatting reasons.
The plan in the snapshot refers to a machine-dependent, so I had tweaked the test logic to replace this path with some constant, but failed to realize this would impact the formatting of the snapshot too (since this is a space-padded table).
--
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: [PR] Reproducer tests for #18380 (resorting sorted inputs) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18352:
URL: https://github.com/apache/datafusion/pull/18352#issuecomment-3482729927
Thanks again @rgehan and @NGA-TRAN
--
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