Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use datafusion::Unparser for converting datafusion Expr to SQL #99

Merged

Conversation

hozan23
Copy link
Contributor

@hozan23 hozan23 commented Sep 14, 2024

Hello,

This PR addresses issue #18.

It removes the basic unparser that was used to convert DataFusion's Expr into SQL and instead uses the implementation from DataFusion's unparser.

Additionally, this PR modifies the API for the SqlTable and SqlExec structs to accept a SQL Dialect, which is used for parsing.

Copy link
Collaborator

@phillipleblanc phillipleblanc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

src/sql/sql_provider_datafusion/mod.rs Outdated Show resolved Hide resolved
src/sql/sql_provider_datafusion/mod.rs Outdated Show resolved Hide resolved
src/sql/sql_provider_datafusion/mod.rs Show resolved Hide resolved
@hozan23
Copy link
Contributor Author

hozan23 commented Sep 14, 2024

Hey @phillipleblanc,

I was also considering the need of Engine, but I'm thinking of wrapping the few Dialects offered by DataFusion with the current Engine enum we have. This way, we can still add some custom logic to these engines and not be restricted by the dialects offered by DataFusion. By doing this, we would no longer need to use Dialect directly, and we could remove it from SqlTable and SqlExec.

@phillipleblanc
Copy link
Collaborator

I think thats fine, but then we should have a mapping from Engine -> Dialect and not require the dialect to be passed in alongside the engine.

@phillipleblanc
Copy link
Collaborator

I pushed up a few commits to implement the Engine -> Dialect mapping and removed passing both Engine & Dialect.

@phillipleblanc phillipleblanc merged commit 58531df into datafusion-contrib:main Sep 15, 2024
3 checks passed
@phillipleblanc
Copy link
Collaborator

Thanks @hozan23!

@hozan23
Copy link
Contributor Author

hozan23 commented Sep 15, 2024

Thanks for merging it @phillipleblanc

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants