phillipleblanc commented on PR #10625: URL: https://github.com/apache/datafusion/pull/10625#issuecomment-2126149300
> I wonder if we should update the comments in `Dialect` so that we can upstream the trait once we have some more experience with what features are needed in DataFusion 🤔 I can see the argument both for upstreaming and for keeping them separate. Although the argument for upstreaming is stronger imo. The argument for upstreaming is: We want a single `Dialect` trait that handles concerns from both parsing and unparsing. That centralizes the logic and makes it so that there is just "one" place to encode dialect-specific differences. 👍 Makes perfect sense. The argument for keeping them separate is: By putting in logic to `sqlparser-rs` that only DataFusion cares about (or more generally, systems that want to handle unparsing) - we set a precedent that "I need this for my use-case, so now everyone is forced to have it even though they don't use it", which I'm not 100% happy with (although I do think its very minor). It would make more sense if (for example) the Dialect was taken into consideration by `sqlparser-rs` when writing out the AST to String, but its only considered on parsing. I'll update the comment for `Dialect`. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
