Projection Expression - Input Field Inconsistencies during Projection #10088

berkaysynnada · 2024-04-15T15:26:39Z

Which issue does this PR close?

Closes #.

Rationale for this change

During projection, 3 cases found where the input field names do not match with the projection expressions:

COUNT(1) expressions appear as COUNT(*) in the input field.
Aggregate function names do not include ORDER BY clause in their field name, but the projections expect them with ORDER BY clause.
While we are reverting first-last value functions, we forgot to convert also ORDER BY clause.

These are not bugs since the projection does not check the names during execution. However, when writing some optimizer rules, projections may be added by looking at the input schema. That inconsistencies cause some errors then.

What changes are included in this PR?

COUNT() expressions are renamed as the way that they appear in the projection.
ScalarFunction, WindowFunction and AggregateFunction's names contain the ORDER BY clause now.
First-Last value functions' order are also reverted now.
While creating the ProjectionMapping, we check the naming of expression and fields.

Are these changes tested?

One test added for the COUNT case, existing tests with new versions cover the other cases.

Are there any user-facing changes?

berkaysynnada · 2024-04-16T07:09:27Z

datafusion/physical-expr/src/equivalence/projection.rs

@@ -67,6 +67,9 @@ impl ProjectionMapping {
                            // Conceptually, `source_expr` and `expression` should be the same.
                            let idx = col.index();
                            let matching_input_field = input_schema.field(idx);
+                            if col.name() != matching_input_field.name() {
+                                return Err(DataFusionError::Internal(format!("Input field name {} does not match with the projection expression {}",matching_input_field.name(),col.name())));
+                            }


This check does not exist before. Now, input field names and projection expressions must match.

ozankabak

LGTM

ozankabak · 2024-04-18T19:16:56Z

@alamb, can you PTAL? This will start generating errors for plans that fail to treat names correctly, so it will have downstream effects

jackwener

A great job to me

jackwener · 2024-04-19T03:24:48Z

datafusion/sqllogictest/test_files/agg_func_substitute.slt

+02)--AggregateExec: mode=FinalPartitioned, gby=[a@0 as a], aggr=[NTH_VALUE(multiple_ordered_table.c,Int64(1)) ORDER BY [multiple_ordered_table.c ASC NULLS LAST]], ordering_mode=Sorted
 03)----SortExec: expr=[a@0 ASC NULLS LAST]
 04)------CoalesceBatchesExec: target_batch_size=8192
 05)--------RepartitionExec: partitioning=Hash([a@0], 4), input_partitions=4
-06)----------AggregateExec: mode=Partial, gby=[a@0 as a], aggr=[NTH_VALUE(multiple_ordered_table.c,Int64(1))], ordering_mode=Sorted
+06)----------AggregateExec: mode=Partial, gby=[a@0 as a], aggr=[NTH_VALUE(multiple_ordered_table.c,Int64(1)) ORDER BY [multiple_ordered_table.c ASC NULLS LAST]], ordering_mode=Sorted


alamb · 2024-04-22T19:31:24Z

datafusion/functions-aggregate/src/first_last.rs

@@ -895,6 +891,31 @@ fn convert_to_sort_cols(
        .collect::<Vec<_>>()
 }

+fn replace_order_by_clause(order_by: &mut String) {


Rather than doing string manipulation here, maybe we could call create_function_physical_name to just create the right name in the first place 🤔

I see -- create_function_physical_name doesn't have sufficient information (Exprs etc to do this)

I suppose the alternate is to remember the relevant parts of the expression, but that also seems brittle.

I can't think of anything better at the moment

alamb

I think erroring when there is a mismatch between physical schemas is a reasonable things to do (it is probably better to fail fast than some other harder to diagnose error later on)

alamb · 2024-04-22T19:35:02Z

datafusion/functions-aggregate/src/first_last.rs

@@ -895,6 +891,31 @@ fn convert_to_sort_cols(
        .collect::<Vec<_>>()
 }

+fn replace_order_by_clause(order_by: &mut String) {


I see -- create_function_physical_name doesn't have sufficient information (Exprs etc to do this)

I suppose the alternate is to remember the relevant parts of the expression, but that also seems brittle.

I can't think of anything better at the moment

alamb · 2024-04-22T19:35:42Z

Thanks @berkaysynnada

…apache#10088) * agg fixes * test updates * fixing count mismatch * Update aggregate_statistics.rs * catch different names * minor

# Description Updates the arrow and datafusion dependencies to 52 and 39(-rc1) respectively. This is necessary for updating pyo3. While most changes with trivial, some required big rewrites. Namely, the logic for the Updates operation had to be rewritten (and simplified) to accommodate some new sanity checks inside datafusion: (apache/datafusion#10088). Depends on delta-kernel having its arrow and object-store version bumped as well. This PR doesn't include any major changes for pyo3, I'll open a separate PR depending on this PR. # Related Issue(s)  # Documentation  --------- Co-authored-by: R. Tyler Croy <rtyler@brokenco.de>

berkaysynnada added 7 commits April 15, 2024 16:09

agg fixes

db9f588

test updates

d5cf921

fixing count mismatch

3d056ec

Merge branch 'apache_main' into bugfix/inconsistency-field-expr

cba4b1f

Update aggregate_statistics.rs

e23bdd1

catch different names

c9e7292

Merge branch 'apache_main' into bugfix/inconsistency-field-expr

78a83a0

github-actions bot added physical-expr Physical Expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Apr 15, 2024

berkaysynnada commented Apr 16, 2024

View reviewed changes

minor

901c639

ozankabak approved these changes Apr 16, 2024

View reviewed changes

jackwener approved these changes Apr 19, 2024

View reviewed changes

alamb reviewed Apr 22, 2024

View reviewed changes

alamb approved these changes Apr 22, 2024

View reviewed changes

alamb merged commit 0780438 into apache:main Apr 22, 2024
24 checks passed

viirya mentioned this pull request Jun 1, 2024

Incorrect input schema when preparing result expressions for HashAggregation apache/datafusion-comet#502

Closed

abhiaagarwal mentioned this pull request Jun 9, 2024

chore: bump to datafusion 39, arrow 52, pyo3 0.21 delta-io/delta-rs#2581

Merged

jonahgao mentioned this pull request Jun 25, 2024

regression: query error with case when as the group field #11118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projection Expression - Input Field Inconsistencies during Projection #10088

Projection Expression - Input Field Inconsistencies during Projection #10088

berkaysynnada commented Apr 15, 2024

berkaysynnada Apr 16, 2024

ozankabak left a comment

ozankabak commented Apr 18, 2024

jackwener left a comment

jackwener Apr 19, 2024

alamb Apr 22, 2024

alamb Apr 22, 2024

alamb left a comment

alamb Apr 22, 2024

alamb commented Apr 22, 2024

Projection Expression - Input Field Inconsistencies during Projection #10088

Projection Expression - Input Field Inconsistencies during Projection #10088

Conversation

berkaysynnada commented Apr 15, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

berkaysynnada Apr 16, 2024

Choose a reason for hiding this comment

ozankabak left a comment

Choose a reason for hiding this comment

ozankabak commented Apr 18, 2024

jackwener left a comment

Choose a reason for hiding this comment

jackwener Apr 19, 2024

Choose a reason for hiding this comment

alamb Apr 22, 2024

Choose a reason for hiding this comment

alamb Apr 22, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Apr 22, 2024

Choose a reason for hiding this comment

alamb commented Apr 22, 2024