Expand handling of LIMIT 1, 2 handling to include sqlite
#1447
Conversation
Pull Request Test Coverage Report for Build 11095970376Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Thank you @joshuawarner32 -- this looks like a very nice improvement. I left some comments but I don't think any is needed to merge this PR
FYI @iffyio
src/parser/mod.rs
Outdated
| @@ -8715,7 +8715,7 @@ impl<'a> Parser<'a> { | |||
| offset = Some(self.parse_offset()?) | |||
| } | |||
|
|
|||
| if dialect_of!(self is GenericDialect | MySqlDialect | ClickHouseDialect) | |||
| if dialect_of!(self is GenericDialect | MySqlDialect | ClickHouseDialect | SQLiteDialect) | |||
There was a problem hiding this comment.
Something else we have been discussing is moving from this dialect_of macro to a method on the Dialect trait instead (the current code uses both patterns)
So it would look something like
if self.dialect.supports_limit_comma() ...The benefits are that user defined Dialects can control the behavior and that there is a better place for documenting the behavior
Maybe something to think about for a future PR
There was a problem hiding this comment.
Makes sense to me! Super easy, so will include this in the next commit to this PR.
src/test_utils.rs
Outdated
| Box::new(DuckDbDialect {}) as Box<dyn Dialect>, | ||
| Box::new(DatabricksDialect {}) as Box<dyn Dialect>, | ||
| ]; | ||
| specific_dialects(vec![ |
src/test_utils.rs
Outdated
| } | ||
|
|
||
| /// Returns a specific set of dialects. | ||
| pub fn specific_dialects(dialects: Vec<Box<dyn Dialect>>) -> TestedDialects { |
There was a problem hiding this comment.
This could also potentially be a constructor on TestedDialects
So like
impl TestedDialects {
pub fn new(dialects: Vec<Box<dyn Dialect>>) -> Self {
Self {
dialects,
options: None,
}
}
}Perhaps
tests/sqlparser_common.rs
Outdated
| @@ -6795,7 +6795,8 @@ fn parse_create_view_with_options() { | |||
| #[test] | |||
| fn parse_create_view_with_columns() { | |||
| let sql = "CREATE VIEW v (has, cols) AS SELECT 1, 2"; | |||
| match verified_stmt(sql) { | |||
| // TODO: why does this fail for ClickHouseDialect? | |||
There was a problem hiding this comment.
Could you perhaps file a ticket to track this isse?
| @@ -8587,17 +8588,26 @@ fn verified_expr(query: &str) -> Expr { | |||
|
|
|||
| #[test] | |||
| fn parse_offset_and_limit() { | |||
| let sql = "SELECT foo FROM bar LIMIT 2 OFFSET 2"; | |||
| let sql = "SELECT foo FROM bar LIMIT 1 OFFSET 2"; | |||
| @@ -60,7 +60,7 @@ use alloc::boxed::Box; | |||
|
|
|||
| /// Convenience check if a [`Parser`] uses a certain dialect. | |||
| /// | |||
| /// `dialect_of!(parser Is SQLiteDialect | GenericDialect)` evaluates | |||
| /// `dialect_of!(parser is SQLiteDialect | GenericDialect)` evaluates | |||
…ts_limit_comma, add reference to newly discovered ClickHouseDialect issue
|
Thanks again @joshuawarner32 and @iffyio |
Sqlite itself parses this syntax, so this parser should parse it when operating in sqlite mode.
Tangential fixups:
all_dialects(), so I rectified that. This required excluding ClickhouseDialect from one existing test, but it works everywhere else.TestedDialects::new(...)function to more cleanly be able to specify exactly the dialects this LIMIT comma syntax works for, in testsLIMIT 1, 2isin doc comment fordialect_of!macro