Skip to content

refactor: add dialect enum #18043

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: add dialect enum #18043

wants to merge 1 commit into from
+141 −35

Conversation

dariocurr
Copy link

@dariocurr dariocurr commented Oct 13, 2025 •
edited
Loading

Which issue does this PR close?

Rationale for this change

This PR introduces a new dialect enum to improve type safety and code maintainability when handling different SQL dialects in DataFusion

  1. Provide compile-time guarantees for dialect handling
  2. Improve code readability and self-documentation
  3. Enable better IDE support and autocomplete

What changes are included in this PR?

  • Added a new Dialect enum to represent supported SQL dialects
  • Refactored existing code to use the new enum instead of previous representations
  • Modified tests to work with the new enum-based approach

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, this is an API change: the type of the dialect field changed from String to Dialect

@github-actions github-actions bot added core Core DataFusion crate common Related to common crate labels Oct 13, 2025
alamb
alamb reviewed Oct 14, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @dariocurr this is a great first contribution. I left some suggestions, let us know what you think

datafusion/core/src/execution/session_state.rs
///
/// [`SessionContext::sql`]: crate::execution::context::SessionContext::sql
#[cfg(feature = "sql")]
pub fn sql_to_statement(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is technically a breaking API change (now it requires a &Dialect). Maybe can you add a note to the upgrade guide and a doc example of how to use this API to help people upgrade?

This is explained a bit more in https://datafusion.apache.org/contributor-guide/api-health.html#upgrade-guides

Copy link
Author

Choose a reason for hiding this comment

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

I added a section in upgrading.md but I’m not quite sure I understand what I need to do. I saw in the datafusion API health guide that "when making breaking public API changes, please add the api-change label to the PR" 

Should I add it something more?

datafusion/common/src/config.rs
}
}

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add documentation to this enum that explains:

  1. What it represents
  2. How it is related to https://docs.rs/sqlparser/latest/sqlparser/dialect/trait.Dialect.html ?

Specifically it would help to understand if the intent is to mirror the code in sqlparser (but is replicated to avoid adding a sqlparser dependency)

Copy link
Author

@dariocurr dariocurr Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your feedback, I added documentation

@github-actions github-actions bot added documentation Improvements or additions to documentation development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) proto Related to proto crate functions Changes to functions implementation ffi Changes to the ffi crate physical-plan Changes to the physical-plan crate spark and removed documentation Improvements or additions to documentation development-process Related to development process of DataFusion logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) proto Related to proto crate functions Changes to functions implementation ffi Changes to the ffi crate physical-plan Changes to the physical-plan crate spark labels Oct 15, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

common Related to common crate core Core DataFusion crate documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: add dialect enum for sql dialects

2 participants

@dariocurr @alamb