Fixes 3 bugs during serialization and deserialization of physical plans #16858

NGA-TRAN · 2025-07-22T20:48:21Z

Which issue does this PR close?

Closes Only 4 tpc-h queries have matching physical plans before serialization and after deserialization #16772

Rationale for this change

Serialization and deserialization of physical plans are currently incomplete. This PR introduces the necessary fixes to ensure all TPC-H queries pass.

What changes are included in this PR?

Summary of Changes:

Introduced pretty_assertions in the proto crate to highlight test output differences more clearly. This addition aids debugging, though I’m open to reverting it if it noticeably impacts crate size.
Resolved the missing aggregate in human_display by adding a corresponding field to the proto definition.
Corrected the date_part display function by introducing a new proto field to handle it properly.
Fixed the issue of extra [] in non-group-by aggregates by checking for empty group-by conditions.
Enabled roundtrip tests for all TPC-H queries to ensure correctness and completeness.

Are these changes tested?

Yes

Added three targeted tests—one for each case—using minimal queries to isolate issues effectively.
You're encouraged to copy these tests into your main branch before applying the fix to confirm that they fail as expected.
Also enabled all currently available TPC-H tests to ensure comprehensive coverage.

Are there any user-facing changes?

The explain for the roundtrip query is now more accurate

NGA-TRAN · 2025-07-22T20:51:54Z

datafusion/proto/proto/datafusion.proto

  bool distinct = 3;
  bool ignore_nulls = 6;
  optional bytes fun_definition = 7;
+  string human_display = 8;


Fix for bug 1 that does not serialize human_display for aggregate function

NGA-TRAN · 2025-07-22T20:52:37Z

datafusion/proto/proto/datafusion.proto

  optional bytes fun_definition = 3;
  datafusion_common.ArrowType return_type = 4;
  bool nullable = 5;
+  string return_field_name = 6;


Fix for bug 3 that does not serialize ScarlarFunction name

NGA-TRAN · 2025-07-22T20:53:12Z

datafusion/proto/tests/cases/roundtrip_physical_plan.rs

-// Only 4 queries pass: q3, q5, q10, q12
-// Ignore the test until the bug is fixed
-#[ignore]
+// Bugs: https://github.com/apache/datafusion/issues/16772


Turn on all 22 tpc-h queries

NGA-TRAN · 2025-07-22T20:54:09Z

datafusion/core/src/physical_planner.rs

            }
+        } else if group_expr.is_empty() {
+            // No GROUP BY clause - create empty PhysicalGroupBy
+            Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))


One of the places that fixes bug 2 that should return nothing for non-groupby

I wonder if we should look into changing group_expr: Option<&[Expr]> or something so the compiler makes it more likely that we handle the difference between an emptyVecvsNone`

@alamb : I had a look. This would be very challenging and a lot of changes needed in physical plan, logical plan, parsing logic, serialization/deserialization. I do not think we want to make a big change just for this

Maybe something we can ask an AI agent to try in a follow on PR

I did, and those were the results I got. It even flagged a semantic error due to not distinguishing between no group and an empty one. Coding without AI these days feels almost impossible. 😄

LiaCastaneda · 2025-07-23T08:23:04Z

datafusion/proto/src/physical_plan/from_proto.rs

+                    Field::new(
+                        &e.return_field_name,
+                        convert_required!(e.return_type)?,
+                        true,


I wonder if this field could ever be non nullable, for example for udfs defined by DF users, then the deserialization would result in a mismatch (not in the scope of this fixes though)

The change here is to use e.return_field_name in deserialization. So whatever name of the UDF we serialize at line 356 of the file to_proto.rs in this PR, it will be deserialized here; and it will match. Does this answer your concern?

I was refering to the nullability of the return Field, that is always being set to true (even before this PR) i was wondering if this could be an issue later if the output of the scalar function could contain nulls, but is unrelated to the changes & fixes of this PR, sorry for the confusion 😄

LiaCastaneda

These are nice finds, thanks! 🙇‍♀️

NGA-TRAN · 2025-07-24T14:40:13Z

@alamb : When you have a moment, could you take a quick look? Thanks!

alamb

This looks like a good change to me -- thank you @NGA-TRAN and @LiaCastaneda

alamb · 2025-07-24T14:52:57Z

datafusion/core/src/physical_planner.rs

            }
+        } else if group_expr.is_empty() {
+            // No GROUP BY clause - create empty PhysicalGroupBy
+            Ok(PhysicalGroupBy::new(vec![], vec![], vec![]))


I wonder if we should look into changing group_expr: Option<&[Expr]> or something so the compiler makes it more likely that we handle the difference between an emptyVecvsNone`

alamb · 2025-07-25T14:32:27Z

Thanks again everyone!

…ns (apache#16858)

Fixes 3 bugs during serialization and deserialization of physical plans

0521f3b

github-actions bot added core Core DataFusion crate proto Related to proto crate ffi Changes to the ffi crate physical-plan Changes to the physical-plan crate labels Jul 22, 2025

NGA-TRAN commented Jul 22, 2025

View reviewed changes

LiaCastaneda reviewed Jul 23, 2025

View reviewed changes

LiaCastaneda approved these changes Jul 23, 2025

View reviewed changes

alamb approved these changes Jul 24, 2025

View reviewed changes

alamb merged commit a6d4798 into apache:main Jul 25, 2025
29 checks passed

adriangb pushed a commit to pydantic/datafusion that referenced this pull request Jul 28, 2025

Fixes 3 bugs during serialization and deserialization of physical pla…

51b65f2

…ns (apache#16858)

Fixes 3 bugs during serialization and deserialization of physical plans #16858

Fixes 3 bugs during serialization and deserialization of physical plans #16858

Uh oh!

Conversation

NGA-TRAN commented Jul 22, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NGA-TRAN Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NGA-TRAN Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiaCastaneda left a comment

Choose a reason for hiding this comment

Uh oh!

NGA-TRAN commented Jul 24, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Jul 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NGA-TRAN Jul 22, 2025 •

edited

Loading

NGA-TRAN Jul 24, 2025 •

edited

Loading