andygrove opened a new issue, #24386: URL: https://github.com/apache/datafusion/issues/24386
### 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`/`new` signatures. 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: | | `FilterExecBuilder` | `HashJoinExecBuilder` | `NestedLoopJoinExecBuilder` | `AnalyzeExecBuilder` | |---|---|---|---|---| | Internal repr | mirrored fields | wrapped `exec` + `preserve_properties` | mirrored fields | mirrored fields | | `build()` returns | `Result<X>` | `Result<X>` (+ `build_exec()` -> `Result<Arc<dyn ExecutionPlan>>`) | `Result<X>` | `X`, infallible | | `From<&X>` | yes (`filter.rs:225`) | yes (`exec.rs:520`) | yes (`nested_loop_join.rs:312`) | no | | `X::builder()` | no | yes, `(&self) -> Builder` (`exec.rs:852`) | no | yes, `(verbose, show_statistics, input, schema)` (`analyze.rs:136`) | | Projection setter | `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: 1. **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 #20276. It does require every setter author to remember to invalidate the flag. 2. **`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. 3. **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. 4. **Projection semantics.** Pick compose or replace, and name the two behaviors distinctly if both are genuinely needed. 5. **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. ### Additional context Related: #20276, #19893, #23708. 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
