adriangb commented on PR #18739: URL: https://github.com/apache/datafusion/pull/18739#issuecomment-3622789290
> We can also take discussion about format of this PR as well, lack description or justification of breaking public API The lack of description in this PR is my fault, I apologize for that. I don't think there's discussion needed, a good PR description is clearly helpful and not having one in this case was frustrating to you and possibly other community members. This is not a justification but this was a follow up / alternative to https://github.com/apache/datafusion/pull/18168 and although it is linked to it in various ways (it's the first link that appears under the discussion) I should have copied the link into the description and added some more background. That's a mistake on my end and I understand why that might be frustrating, but I don't think that invalidates the entire change or requires the PR to be reverted. https://github.com/apache/datafusion/pull/18168#issuecomment-3536298767 (from the PR mentioned above) links to code currently being used to work around this that is very similar to what you are proposing: ```rust impl ConfigExtension for DistributedConfig { const PREFIX: &'static str = "distributed"; } ``` While it technically works, it is not ergonomic. My view and understanding of the project is that wanting to do something with DataFusion that ends up being hard to figure out / not ergonomic is generally a valid reason for making tweaks to a public API. Beyond that I think the details of what constitutes unergonomic and what is a large or small API change are somewhat subjective. Gabriel and I do not work for the same organization, so there is no `you` here. We looked at the problem, had a PR with a good description and justification open for over a month and then came to the conclusion together that this would be the best solution. I understand that is maybe irrelevant if it's easy to miss because it's not linked to from the PR description. I hope you find this valid reasoning for the change and I once again apologize for not putting this more succinctly in the PR description. If you still are not comfortable with the change I think we should just revert it and start discussion anew with more formal information in the PR description itself. It doesn't seem worth burdening the wider community with. Would you like me to revert the PR? -- 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]
