Relax constraint that file sort order must only reference individual columns #17419
+258
−89
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
physical-expr
pepijnve
force-pushed
the
issue_17411
branch
7 times, most recently
from
4e04412 to
4bbc81a
Compare
pepijnve
force-pushed
the
issue_17411
branch
from
49a6c8a to
5861425
Compare
pepijnve
force-pushed
the
issue_17411
branch
from
5861425 to
9453640
Compare
alamb
approved these changes
Sep 10, 2025
datafusion/catalog/src/stream.rs
Outdated
| } | ||
| None => create_ordering(self.0.source.schema(), &self.0.order)?, | ||
| let schema = self.0.source.schema(); | ||
| let df_schema = DFSchema::try_from(Arc::clone(schema))?; |
There was a problem hiding this comment.
I wonder if (re)creating this DFSchema is necessary -- it feels like at this point we know the schema information
However, i also see we need to have a DFSchema to correctly create arbitrary PhysicalExprs so this is probably fine
There was a problem hiding this comment.
I was a bit concerned about the waste here as well, but I couldn't figure out a simple way to avoid this.
| ---- | ||
| physical_plan DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/sqllogictest/data/composite_order.csv]]}, projection=[a, b], output_ordering=[a@0 + b@1 ASC NULLS LAST], file_type=csv, has_header=true | ||
|
|
||
| # Query ordered by the declared order should be just a table scan |
| 02)--CoalesceBatchesExec: target_batch_size=4096 | ||
| 03)----FilterExec: b@2 = 0 | ||
| 04)------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a0, a, b, c, d], output_orderings=[[a@1 ASC NULLS LAST, b@2 ASC NULLS LAST], [c@3 ASC NULLS LAST]], file_type=csv, has_header=true | ||
| 04)------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a0, a, b, c, d], output_orderings=[[c@3 ASC NULLS LAST], [a@1 ASC NULLS LAST, b@2 ASC NULLS LAST]], file_type=csv, has_header=true |
There was a problem hiding this comment.
do you know why the output orderings come out in a different (reverse) order now?
There was a problem hiding this comment.
No, I didn't take the time to try to understand why. That's how they're being emitted by the EquivalenceClass code. I had assumed the order was not important, but if it is I can take a closer look.
There was a problem hiding this comment.
i don't think it is important
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
|
I am a little worried about the reported slowdowns in the sql planning benchmarks. I'll try and reproduce them locally |
|
Perhaps a two step approach would be better then where we try the “column only” version first and only use the more complex code path as fallback. |
This would have the very nice property that it was at least as fast as the current implementation (aka no regressions) and we can always try and make the performance faster for the newly supported expressions with a follow on PR |
|
I had quick look using flamegraph at |
pepijnve
force-pushed
the
issue_17411
branch
6 times, most recently
from
5aafdca to
10dff75
Compare
pepijnve
force-pushed
the
issue_17411
branch
from
10dff75 to
d8de8db
Compare
…e individual columns
pepijnve
force-pushed
the
issue_17411
branch
from
d8de8db to
d13b525
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
@alamb still no luck with the planner benchmarks it seems |
It appears to be blowing up due to the newly added planning benchmarks for window functions. Conveniently, this appears to be almost fixed with Once that is merged, I'll try (again) and hopefully succeed this time |
|
I merged this branch up from main to get the fix for #17684 and will try again to get the benchmarks to run |
|
🤖 |
|
Aha -- the benchmark is not running because it is panic'ing on this branch: To reproduce: cargo bench --profile dev --bench sql_planner -- physical_plan_tpcds_allThis passes on main It panic's on this branch like this: Benchmarking physical_plan_tpcds_all: Warming up for 3.0000 s
thread 'main' panicked at datafusion/core/benches/sql_planner.rs:64:14:
called `Result::unwrap()` on an `Err` value: Internal("Physical input schema should be the same as the one converted from logical input schema. Differences: \n\t- field nullability at index 5 [sales_cnt]: (physical) true vs (logical) false\n\t- field nullability at index 6 [sales_amt]: (physical) true vs (logical) false")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtraceThat is somewhat bad that all the tests pass too 🤔 |
|
EDIT -- the same test fails on main too (not related to this PR it seems) |
|
Thanks for checking already. I’ll see if I can find some time to get that fixed on main since we have no way to do comparative testing otherwise. |
This was referenced Sep 26, 2025
|
Here is a PR to temporarily disable the failing test |
xudong963
approved these changes
Sep 28, 2025
Comment on lines
+241
to
+243
| // Err result indicates an expression could not be found in the | ||
| // projected_schema, stop iterating since rest of the orderings are violated | ||
| break; |
There was a problem hiding this comment.
Should we give a warn!() here?
There was a problem hiding this comment.
It's not really an error case. This happens, for instance, when the input has sort order a, b, but the projection of the node only retains a. The resulting order is just a.
I'm kind of abusing Err here. The plan_err!("") bit is basically just there to abort the transform_up tree walk early since there's no point in continuing. The Err(_) bit is handling it. Not pretty, but I couldn't find a nicer way to achieve the desired result.
FWIW, this is the same behaviour as the code that's already present.
|
Merging up from main to get #17809 |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
@alamb seems like there are still some small increases here and there, but much better than before. Is it worth looking into these still or is this within the acceptable range? I can dig deeper, but I could use some guidance on how best to profile the benchmarks. Last time I tried I seemed to be getting nothing but noise and framework overhead in the flame graph. |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
I am looking at this a little more closely nwo |
alamb
approved these changes
Oct 1, 2025
There was a problem hiding this comment.
I went back and reviewed this PR carefully and I don't think it causes any slowdowns. I also spent some time profiling locally using
cargo bench --profile=profiling --bench sql_planner -- --bench physical_plan_clickbench_q43And could not see any difference
I also then prevented inlining of the potential functions
index a4ed8187c..bd73b5488 100644
--- a/datafusion/physical-expr/src/equivalence/projection.rs
+++ b/datafusion/physical-expr/src/equivalence/projection.rs
@@ -211,6 +211,7 @@ pub fn project_orderings(
/// schema only contains b and not a. The result will be `Some([col("a@0")])`. In other
/// words, the column reference is reindexed to match the projected schema.
/// If neither a nor b is present, the result will be None.
+#[inline(never)]
pub fn project_ordering(
ordering: &LexOrdering,
schema: &SchemaRef,
diff --git a/datafusion/physical-expr/src/physical_expr.rs b/datafusion/physical-expr/src/physical_expr.rs
index 2cc484ec6..02b75a0b8 100644
--- a/datafusion/physical-expr/src/physical_expr.rs
+++ b/datafusion/physical-expr/src/physical_expr.rs
@@ -163,6 +163,7 @@ pub fn create_ordering(
}
/// Creates a vector of [LexOrdering] from a vector of logical expression
+#[inline(never)]
pub fn create_lex_ordering(
schema: &SchemaRef,
sort_order: &[Vec<SortExpr>],ANd didn't see them show up any any of the traces when I ran it like
samply record -- target/profiling/deps/sql_planner-1adcb045f71bd635 --bench physical_plan_clickbench_q43
Thus I conclude this PR is good to go and I am going to merge it in.
Thank you for your patience and help @pepijnve
I'll try samply next time. Was that on your macOS machine or on Linux? I tried essentially the same using the flamegraph crate, but the graph I got out of that (on macOS at least) didn't make much sense to me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The documentation states that
WITH ORDERclauses may use non-trivial expressions. It even has an example showing the usage of this feature. In practice this does not work and the implementation is limited to simple column references.What changes are included in this PR?
physical_expr::create_lex_orderingfunction that provides a more flexible version ofphysical_expr::create_ordering.create_orderingwith its single column constraint has been retained for backwards compatibility, but should perhaps be deprecated. It does not seems possible to reimplement it in terms ofcreate_lex_orderingsince anExecutionPropsinstance is required.physical_expr::equivalence::project_orderingsconvenience function that uses the existing sort order projection logicAre these changes tested?
with ordercaseAre there any user-facing changes?
PhysicalExprinstances rather than onlyColumn.