Skip to content

refactor: add try_to_proto / try_from_proto to DynamicFilterPhysicalExpr#22452

Open
AnuragRaut08 wants to merge 2 commits into
apache:mainfrom
AnuragRaut08:refactor/dynamic-filter-proto
Open

refactor: add try_to_proto / try_from_proto to DynamicFilterPhysicalExpr#22452
AnuragRaut08 wants to merge 2 commits into
apache:mainfrom
AnuragRaut08:refactor/dynamic-filter-proto

Conversation

@AnuragRaut08
Copy link
Copy Markdown

Which issue does this PR close?

Part of #22434

What changes are included?

Adds try_to_proto and try_from_proto to DynamicFilterPhysicalExpr
so it participates in the expression-local serialization pattern
introduced in #21929.

The centralized arms in to_proto.rs / from_proto.rs remain as
fallbacks for now. Cleanup of the pub-for-proto scaffolding
(from_parts, inner, original_children, remapped_children) can
follow in a separate PR once decode reads state directly.

Are these changes tested?

Yes — all three existing dynamic filter roundtrip tests pass:

  • test_dynamic_filter_roundtrip_dedupe
  • test_dynamic_filter_plan_roundtrip_dedupe
  • test_dynamic_filter_expression_id_is_stable_between_serializations

Are there any user-facing changes?

No.

Part of apache#22434. Adds try_to_proto / try_from_proto to
DynamicFilterPhysicalExpr so it participates in the expression-local
serialization pattern introduced in apache#21929.

The centralized arms in to_proto.rs / from_proto.rs remain as fallbacks
for now. Cleanup of the pub-for-proto scaffolding (from_parts, inner,
original_children, remapped_children) can follow once decode reads
state directly via try_from_proto.
@github-actions github-actions Bot added the physical-expr Changes to the physical-expr crates label May 22, 2026
@AnuragRaut08
Copy link
Copy Markdown
Author

All three existing dynamic filter roundtrip tests pass locally:

  • test_dynamic_filter_roundtrip_dedupe
  • test_dynamic_filter_plan_roundtrip_dedupe
  • test_dynamic_filter_expression_id_is_stable_between_serializations

Happy to iterate quickly on any feedback. Also open to tackling the pub-for-proto scaffolding cleanup (from_parts, inner, original_children, remapped_children) as a follow-up PR once this lands, if that works for the maintainers.

@jayshrivastava
Copy link
Copy Markdown
Contributor

Do you want to delete the public methods in this PR? I reviewed your current commit and it looks good.

Here's a commit removing the public methods you may want to cherry-pick.
52b1786

///
/// Encodes `children`, `remapped_children`, and the atomically-captured
/// `Inner` state (expression id, generation, current expr, is_complete).
pub fn try_to_proto(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this path is being used. I think it needs to move to the impl PhysicalExpr block to override the default PhysicalExpr::try_to_proto implementation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnuragRaut08 are you able to move these methods and add tests?

@adriangb
Copy link
Copy Markdown
Contributor

#22596 adds an expect_expr_variant! macro and a require_proto_field helper that together collapse the outer match and any non-expression "missing required field" ok_or_else to one line each. Could you rebase onto it and adopt them before final review?

@AnuragRaut08
Copy link
Copy Markdown
Author

Thanks, I’m working through the rebase now.

After rebasing onto latest main, the module moved from dynamic_filters.rs to dynamic_filters/mod.rs, so I’m updating the implementation to follow the newer migrated try_to_proto / try_from_proto pattern used by the other physical expressions.

I’ve moved try_to_proto into the impl PhysicalExpr block and am now updating the decode path + tests. I’ll also rebase onto #22596 and adopt the new helpers before pushing the next revision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants