You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/10/17 00:32:41 UTC
[PR] Allow a SessionState to carry a custom sqlparser dialect [datafusion]
adriangb opened a new pull request, #26437:
URL: https://github.com/apache/datafusion/pull/26437
## Which issue does this PR close?
- Closes https://github.com/apache/datafusion/issues/23109.
This picks up the approach of https://github.com/apache/datafusion/pull/23235, which the stale bot closed without a review decision. This PR adds the clone-path, `SET` and expression-path coverage, and documents the precedence rule.
## Rationale for this change
`datafusion.sql_parser.dialect` accepts only a closed list of names. `SessionState::sql_to_statement` and `SessionState::sql_to_expr_with_alias` resolve that name with `dialect_from_str` on every parse. Thus an application with its own `sqlparser::dialect::Dialect`, for example one built with `derive_dialect!` on top of `PostgreSqlDialect` (https://github.com/apache/datafusion-sqlparser-rs/pull/2174), cannot plug it in. It must wrap every parse entry point itself. A call to a stock method such as `SessionContext::sql` then parses with the wrong dialect, and the query fails with a parse error.
## What changes are included in this PR?
- New `SessionStateBuilder::with_sql_dialect(Arc<dyn Dialect + Send + Sync>)`, plus the getters `SessionState::sql_dialect()` and `SessionStateBuilder::sql_dialect()`.
- `sql_to_statement` and `sql_to_expr_with_alias` use the dialect object when it is set. All other parse paths go through these two methods: `SessionContext::sql`, `sql_with_options`, `create_logical_plan`, `create_logical_expr` and `sql_to_expr`.
- Precedence: the dialect object wins over the dialect name. This includes an explicit `dialect` argument to `sql_to_statement` or `sql_to_expr`, and a later `SET datafusion.sql_parser.dialect = ...`. `SET` still changes the config value, but not the parser.
- `SessionStateBuilder::new_from_existing` and `From<SessionState>` keep the dialect object.
- If no dialect object is set, the behavior does not change.
`datafusion-cli` resolves the dialect name itself and does not expose a way to set a dialect object, so this PR does not change it.
## Are these changes tested?
Yes. `test_custom_sql_dialect` uses a small hand-written dialect: PostgreSQL plus `SELECT * EXCEPT (...)`. The `derive-dialect` feature of sqlparser is not enabled in this workspace. The test checks that:
- the PostgreSQL dialect name alone rejects the query (default behavior unchanged),
- `ctx.sql(...)`, `create_logical_plan`, `sql_to_statement`, `sql_to_expr` and `create_logical_expr` parse with the dialect object,
- a `SET datafusion.sql_parser.dialect` statement does not replace it,
- `new_from_existing` and `From<SessionState>` keep it.
The rustdoc example on `with_sql_dialect` runs as a doctest.
## Are there any user-facing changes?
Yes. This PR adds a new public builder method and two getters. There are no breaking changes.
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allow a SessionState to carry a custom sqlparser dialect [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26437:
URL: https://github.com/apache/datafusion/pull/26437#issuecomment-6227648393
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26437?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `77.64706%` with `19 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.89%. Comparing base ([`5f21582`](https://app.codecov.io/gh/apache/datafusion/commit/5f2158269e24ac45e2740dcfe6e79a6f69f28330?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`89c8cc7`](https://app.codecov.io/gh/apache/datafusion/commit/89c8cc7ad0264df81f494c974c32be1cf279ab36?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26437?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/core/src/execution/session\_state.rs](https://app.codecov.io/gh/apache/datafusion/pull/26437?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Fexecution%2Fsession_state.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9leGVjdXRpb24vc2Vzc2lvbl9zdGF0ZS5ycw==) | 77.64% | [6 Missing and 13 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26437?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26437 +/- ##
==========================================
- Coverage 82.89% 82.89% -0.01%
==========================================
Files 1154 1154
Lines 457620 457698 +78
Branches 457620 457698 +78
==========================================
+ Hits 379352 379415 +63
- Misses 55262 55265 +3
- Partials 23006 23018 +12
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26437?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allow a SessionState to carry a custom sqlparser dialect [datafusion]
Posted by "alamb (via GitHub) via github" <gi...@datafusion.apache.org>.
alamb commented on code in PR #26437:
URL: https://github.com/apache/datafusion/pull/26437#discussion_r4473190815
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1435,6 +1477,46 @@ impl SessionStateBuilder {
self
}
+ /// Set the [`sqlparser`] dialect object used to parse SQL.
+ ///
+ /// Use this method for a dialect that `datafusion.sql_parser.dialect`
+ /// cannot name, for example a custom implementation of
+ /// [`sqlparser::dialect::Dialect`].
+ ///
+ /// The dialect object takes precedence over the dialect name in the
+ /// session config. If it is set, [`SessionContext::sql`],
Review Comment:
do we really need to list all the (current) users of this setting?
##########
datafusion/core/src/execution/session_state.rs:
##########
@@ -1435,6 +1477,46 @@ impl SessionStateBuilder {
self
}
+ /// Set the [`sqlparser`] dialect object used to parse SQL.
+ ///
+ /// Use this method for a dialect that `datafusion.sql_parser.dialect`
Review Comment:
I think this could be shorter "Use this method for custom dialects that are not supported by ..."
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allow a SessionState to carry a custom sqlparser dialect [datafusion]
Posted by "adriangb (via GitHub) via github" <gi...@datafusion.apache.org>.
adriangb commented on PR #26437:
URL: https://github.com/apache/datafusion/pull/26437#issuecomment-6713040136
I think so! Let me address your feedback next week or feel free to push and merge before that
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Allow a SessionState to carry a custom sqlparser dialect [datafusion]
Posted by "alamb (via GitHub) via github" <gi...@datafusion.apache.org>.
alamb commented on PR #26437:
URL: https://github.com/apache/datafusion/pull/26437#issuecomment-6706816146
Shall we merge it?
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org