You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
apply_projection -> Result<Self>, composes with existing
with_projection, infallible, replaces
with_projection, infallible, replaces
n/a
Two of these are genuine footguns rather than cosmetic drift:
X::builder() means opposite things.HashJoinExec::builder(&self) clones an existing plan into a builder; AnalyzeExec::builder(a, b, c, d) is an associated constructor taking required args. Same name, incompatible signatures.
Projection setters differ in semantics, not just name.FilterExecBuilder::apply_projection composes through an existing projection (current [0, 2, 3] + [0, 2] -> [0, 3]); the join builders' with_projection overwrites. Anyone porting code between operators gets wrong results, not a compile error.
Describe the solution you'd like
Agree on one canonical shape, document it in docs/source/contributor-guide/api-health.md next to the deprecation guidelines (builders are currently undocumented there), then align the four existing builders before extending the pattern further.
Points to settle:
Internal representation. Mirrored fields, or HashJoinExecBuilder's wrapped-exec + preserve_properties flag? The wrapped approach avoids duplicating the field list (adding a field = one setter, not three edits) and recomputes PlanProperties once in build() rather than per-setter — see Improve HashJoinExecBuilder to save state from previous fields #20276. It does require every setter author to remember to invalidate the flag.
build() fallibility. Uniformly Result<X>, even where currently infallible, so a validation can be added later without a breaking change? Also whether build_exec() -> Result<Arc<dyn ExecutionPlan>> should exist on all builders or none.
Getting a builder from an existing plan. Standardize on From<&X>, and either drop X::builder() or fix its meaning to one of the two.
Projection semantics. Pick compose or replace, and name the two behaviors distinctly if both are genuinely needed.
Should required constructor args also get setters?FilterExecBuilder has with_input/with_predicate; the join builders don't.
Describe alternatives you've considered
Keep new(required) + with_* on the exec itself. Adding a private field plus a with_* setter is already non-breaking, so this partly works — but it recomputes PlanProperties on every setter call (e.g. SortExec::with_fetch, sort.rs:1057) and can't validate field combinations, which is what motivated builders in the first place.
Leave the existing four alone and only apply a convention to new builders — cheaper now, but leaves the inconsistency permanently in the public API.
Follow-on work (builders for the remaining operators — SymmetricHashJoinExec and HashJoinExec take 9 constructor args, SortMergeJoinExec and PiecewiseMergeJoinExec 7, AggregateExec and StreamingTableExec 6) should be a separate tracking issue once the shape is settled.
Is your feature request related to a problem or challenge?
We've begun adding builders to physical operators so that new fields can be added without breaking
try_new/newsignatures. Four exist today:FilterExecBuilder(datafusion/physical-plan/src/filter.rs:105)HashJoinExecBuilder(datafusion/physical-plan/src/joins/hash_join/exec.rs:288)NestedLoopJoinExecBuilder(datafusion/physical-plan/src/joins/nested_loop_join.rs:232)AnalyzeExecBuilder(datafusion/physical-plan/src/analyze.rs:72)They were added incrementally and have diverged in ways that will be baked in if we roll the pattern out to the remaining ~30 operators:
FilterExecBuilderHashJoinExecBuilderNestedLoopJoinExecBuilderAnalyzeExecBuilderexec+preserve_propertiesbuild()returnsResult<X>Result<X>(+build_exec()->Result<Arc<dyn ExecutionPlan>>)Result<X>X, infallibleFrom<&X>filter.rs:225)exec.rs:520)nested_loop_join.rs:312)X::builder()(&self) -> Builder(exec.rs:852)(verbose, show_statistics, input, schema)(analyze.rs:136)apply_projection->Result<Self>, composes with existingwith_projection, infallible, replaceswith_projection, infallible, replacesTwo of these are genuine footguns rather than cosmetic drift:
X::builder()means opposite things.HashJoinExec::builder(&self)clones an existing plan into a builder;AnalyzeExec::builder(a, b, c, d)is an associated constructor taking required args. Same name, incompatible signatures.FilterExecBuilder::apply_projectioncomposes through an existing projection (current[0, 2, 3]+[0, 2]->[0, 3]); the join builders'with_projectionoverwrites. Anyone porting code between operators gets wrong results, not a compile error.Describe the solution you'd like
Agree on one canonical shape, document it in
docs/source/contributor-guide/api-health.mdnext to the deprecation guidelines (builders are currently undocumented there), then align the four existing builders before extending the pattern further.Points to settle:
HashJoinExecBuilder's wrapped-exec +preserve_propertiesflag? The wrapped approach avoids duplicating the field list (adding a field = one setter, not three edits) and recomputesPlanPropertiesonce inbuild()rather than per-setter — see ImproveHashJoinExecBuilderto save state from previous fields #20276. It does require every setter author to remember to invalidate the flag.build()fallibility. UniformlyResult<X>, even where currently infallible, so a validation can be added later without a breaking change? Also whetherbuild_exec() -> Result<Arc<dyn ExecutionPlan>>should exist on all builders or none.From<&X>, and either dropX::builder()or fix its meaning to one of the two.FilterExecBuilderhaswith_input/with_predicate; the join builders don't.Describe alternatives you've considered
Keep
new(required) + with_*on the exec itself. Adding a private field plus awith_*setter is already non-breaking, so this partly works — but it recomputesPlanPropertieson every setter call (e.g.SortExec::with_fetch,sort.rs:1057) and can't validate field combinations, which is what motivated builders in the first place.Leave the existing four alone and only apply a convention to new builders — cheaper now, but leaves the inconsistency permanently in the public API.
Additional context
Related: #20276, #19893, #23708.
Follow-on work (builders for the remaining operators —
SymmetricHashJoinExecandHashJoinExectake 9 constructor args,SortMergeJoinExecandPiecewiseMergeJoinExec7,AggregateExecandStreamingTableExec6) should be a separate tracking issue once the shape is settled.