asolimando commented on PR #21122: URL: https://github.com/apache/datafusion/pull/21122#issuecomment-4288222752
> Thanks @asolimando! > > The algorithmic contribution is genuinely useful, and thanks for your patience in keeping at it. > > But the registry-injection plumbing adds three permanent `ExecutionPlan` trait methods and five struct fields to work around a missing parameter that #20184 is poised to add properly. > > Before landing, I'd want alignment on whether to (a) wait for #20184 and land this on top of a `StatisticsContext` parameter, or (b) accept the injection design as a permanent API surface Thanks @xudong963 for your thoughtful feedback, the injection design was meant as a stepping stone, not a permanent API surface, and I totally agree that adding the `StatisticsContext` parameter is the right long term solution, buying us freedom to provide an even richer context in the future. I wanted to mark the new trait methods as "experimental" but I couldn't find a proper mechanism for that in DataFusion. Re. (b), I was under the impression that #20184 might land pretty soon, so I was basically counting on rebasing before finalizing, or shortly after, as breaking changes within the same unreleased version are generally tolerated, but if the timeline to merge #20184 is uncertain, (b) might not be ideal. The cons of (a) is the risk of conflicts requiring to force-push, making it harder for reviewers to check incrementally. In case it makes your decision easier, I can commit on helping with #20184's implementation, under both scenarios, if the current assignee is busy. WDYT? -- 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]
