You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "7phs (via GitHub)" <gi...@apache.org> on 2024/12/03 12:05:22 UTC
[PR] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
7phs opened a new pull request, #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576
Quoted identifiers allow the use of any characters and can start with any character, including digits. For example:
```sql
SELECT 1 AS "1" FROM a
```
However, the tokenizer cannot parse such types of quoted identifiers in the case of Redshift. This issue is related to the handling of potential identifiers in PartiQL.
This merge request (MR) clarifies the behavior of quoted identifiers in PartiQL and fixes the parsing of quoted identifiers for Redshift.
I added an additional case of a valid JSON path in a specific unit test to ensure that JSON path parsing is not broken.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873845800
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +380,23 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ // An alias starting with a number
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1abc" FROM a"#);
+}
+
+#[test]
+fn test_parse_nested_quoted_identifier() {
+ redshift().verified_stmt(r#"SELECT 1 AS ["1"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS [ " 1 " ]"#);
Review Comment:
It depends on what we want to achieve. I have implemented parsing of nested quotations, guided by the test case [brackets_over_db_schema_table_name_with_whites_paces](https://github.com/apache/datafusion-sqlparser-rs/blob/main/tests/sqlparser_redshift.rs#L69). For example, the following query:
```
SELECT [ col1 ] FROM [ test_schema].[ test_table]
```
is successfully parsed using `verified_stmt`.
I don't have a strong opinion on this implementation and can adjust the parsing if needed.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1867993041
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
This method is called by [tokeniser](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/tokenizer.rs#L1078):
```
quote_start
if self.dialect.is_delimited_identifier_start(ch)
&& self
.dialect
.is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
```
It applies to both delimiters of identifiers (`"` and `[`) to determine whether the current character sequence marks the beginning of a quoted token. This code is executed regardless of the specific characters—be it `[0]` or `"a"`.
However, only `[` is considered the start of a quoted identifier in json path case. This PR clarifies this behavior with a more explicit condition at the start of the method.
The highlighted line can only be triggered for `[0]` and never for `"a"`. However, there is a possibility that a double-quoted identifier like `["a"]` could be parsed. An additional check for `"` addresses this scenario. Note that JSON path expressions use single quotes `['a']` and never double quotes.
I added unit-tests to cover all these cases.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1872019394
##########
src/dialect/redshift.rs:
##########
@@ -41,14 +41,48 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
Review Comment:
@iffyio I have updated the PR based on your suggestion. Now, a common quoted identifier is processed without additional conditions, and RedShift uses the same set of starting characters for quoted identifiers as PostgreSQL.
Additionally, I added a specific match to handle the RedShift-specific case for `[`. All tests passed without requiring any 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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1867993041
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
This method is called by [tokeniser](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/tokenizer.rs#L1078):
```
quote_start
if self.dialect.is_delimited_identifier_start(ch)
&& self
.dialect
.is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
```
It applies to both delimiters of identifiers (`"` and `[`) to determine whether the current character sequence marks the beginning of a quoted token. This code is executed regardless of the specific characters—be it `[0]` or `"a"`.
However, only `[` is considered the start of a quoted identifier in json path case. This PR clarifies this behavior with a more explicit condition at the start of the method.
The highlighted line can only be triggered for `[0]` and never for `"a"`. However, there is a possibility that a double-quoted identifier like `["a"]` could be parsed. An additional check for `"` addresses this scenario. Note that JSON path expressions use single quotes `['a']` and never double quotes.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870413689
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,24 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as
+ // a start character and a quote is a beginning of quoted identifier.
+ // Skipping analyzing token such as `"a"` and analyze only token that
+ // can be part of json path potentially.
+ // For ex., `[0]`, `['a']` (seems part of json path) or `["a"]` (normal quoted identifier)
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
Review Comment:
Thank you for feedback!
A goal of this MR is improving implementation of RedShift dialect. The method `is_proper_identifier_inside_quotes` is implemented only for RedShift and I assume that it added only for RedShift specific case.
I verified one more time and found that `SELECT foo["bar"] FROM table_name` or `SELECT foo['bar'] FROM table_name` is not valid in RedShift case. `SELECT foo."bar" FROM table_name` is only valid for this certain case. We don't need to care about this case. I updated comments.
I'm going to improve verification of quoted identifier in this special case that you mentioned. I found that a parsed will fail on this valid SQL `SELECT 1 AS ["]="] FROM a`.
Thank you for highlighting this edge case!
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1868012113
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
I added comments with examples to make a reason of conditions little bit more explicit.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870464718
##########
src/tokenizer.rs:
##########
@@ -1081,19 +1081,34 @@ impl<'a> Tokenizer<'a> {
.dialect
.is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
- let quote_end = Word::matching_end_quote(quote_start);
- let (s, last_char) = self.parse_quoted_ident(chars, quote_end);
+ let word = if let Some(nested_quote_start) = self
+ .dialect
+ .nested_quote_start(quote_start, chars.peekable.clone())
+ {
+ chars.next(); // consume the opening quote
Review Comment:
This code is RedShift specific. The same time tokenisation of quoted identifier is a method of tokeniser and this is a reason of implemented it here.
Let me know if you have a good idea how makes this code more explicit.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "bombsimon (via GitHub)" <gi...@apache.org>.
bombsimon commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1867724836
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
Since the precedence here will group the `&&` expression, why do we need the `ch != '\''`, isn't that already checked in `is_identifier_start`?
Maybe I'm not understanding this part:
> PartiQL uses single quote as starting identification inside a quote
Would that be `"'foo'"`?
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "bombsimon (via GitHub)" <gi...@apache.org>.
bombsimon commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873553041
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,38 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// Example: RedShift supports the following quote styles to all mean the same thing:
+ /// ```sql
+ /// SELECT 1 AS foo;
+ /// SELECT 1 AS "foo";
+ /// SELECT 1 AS [foo];
+ /// SELECT 1 AS ["foo"];
+ /// ```
+ fn is_nested_delimited_identifier_start(&self, _ch: char) -> bool {
+ false
+ }
+
+ /// Only applicable whenever [`Self::is_nested_delimited_identifier_start`] returns true
+ /// If the next sequence of tokens potentially represent a nested identifier, then this method
+ /// returns a tuple containing the outer quote style, and if present, the inner (nested) quote style.
+ ///
+ /// Example (Redshift):
+ /// ```text
+ /// `["foo"]` => (Some(`[`), Some(`"`))
+ /// `[foo]` => (Some(`[`), None)
+ /// `"foo"` => None
Review Comment:
nit: Looks like this doesn't line up with the return type.
```suggestion
/// `["foo"]` => Some(('[', Some('"'))
/// `[foo]` => Some(('[', None))
/// `"foo"` => None
```
##########
src/dialect/redshift.rs:
##########
@@ -32,21 +32,50 @@ pub struct RedshiftSqlDialect {}
// in the Postgres dialect, the query will be parsed as an array, while in the Redshift dialect it will
// be a json path
impl Dialect for RedshiftSqlDialect {
- fn is_delimited_identifier_start(&self, ch: char) -> bool {
- ch == '"' || ch == '['
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// Example: RedShift supports the following quote styles to all mean the same thing:
+ /// ```sql
+ /// SELECT 1 AS foo;
+ /// SELECT 1 AS "foo";
+ /// SELECT 1 AS [foo];
+ /// SELECT 1 AS ["foo"];
+ /// ```
+ fn is_nested_delimited_identifier_start(&self, ch: char) -> bool {
+ ch == '['
}
- /// Determine if quoted characters are proper for identifier
- /// It's needed to distinguish treating square brackets as quotes from
- /// treating them as json path. If there is identifier then we assume
- /// there is no json path.
- fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ /// Only applicable whenever [`Self::is_nested_delimited_identifier_start`] returns true
+ /// If the next sequence of tokens potentially represent a nested identifier, then this method
+ /// returns a tuple containing the outer quote style, and if present, the inner (nested) quote style.
+ ///
+ /// Example (Redshift):
+ /// ```text
+ /// `["foo"]` => (Some(`[`), Some(`"`))
+ /// `[foo]` => (Some(`[`), None)
+ /// `"foo"` => None
Review Comment:
Same here right?
```suggestion
/// `["foo"]` => Some(('[', Some('"'))
/// `[foo]` => Some(('[', None))
/// `"foo"` => None
```
```
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1874399142
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +380,23 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ // An alias starting with a number
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1abc" FROM a"#);
+}
+
+#[test]
+fn test_parse_nested_quoted_identifier() {
+ redshift().verified_stmt(r#"SELECT 1 AS ["1"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS [ " 1 " ]"#);
Review Comment:
I have updated the PR to trim spaces in cases of nested quotations, e.g., `[ "foo" ]` -> `["foo"]`. Implementing similar behavior for cases like `[ foo ]` would require more extensive changes to the existing code. In my opinion, this could introduce some risk, at least in the context of the current PR.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873653672
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,38 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// Example: RedShift supports the following quote styles to all mean the same thing:
+ /// ```sql
+ /// SELECT 1 AS foo;
+ /// SELECT 1 AS "foo";
+ /// SELECT 1 AS [foo];
+ /// SELECT 1 AS ["foo"];
+ /// ```
+ fn is_nested_delimited_identifier_start(&self, _ch: char) -> bool {
+ false
+ }
+
+ /// Only applicable whenever [`Self::is_nested_delimited_identifier_start`] returns true
+ /// If the next sequence of tokens potentially represent a nested identifier, then this method
+ /// returns a tuple containing the outer quote style, and if present, the inner (nested) quote style.
+ ///
+ /// Example (Redshift):
+ /// ```text
+ /// `["foo"]` => (Some(`[`), Some(`"`))
+ /// `[foo]` => (Some(`[`), None)
+ /// `"foo"` => None
Review Comment:
Nice catch.
Updated and I added one more example `[0]`
##########
src/dialect/redshift.rs:
##########
@@ -32,21 +32,50 @@ pub struct RedshiftSqlDialect {}
// in the Postgres dialect, the query will be parsed as an array, while in the Redshift dialect it will
// be a json path
impl Dialect for RedshiftSqlDialect {
- fn is_delimited_identifier_start(&self, ch: char) -> bool {
- ch == '"' || ch == '['
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// Example: RedShift supports the following quote styles to all mean the same thing:
+ /// ```sql
+ /// SELECT 1 AS foo;
+ /// SELECT 1 AS "foo";
+ /// SELECT 1 AS [foo];
+ /// SELECT 1 AS ["foo"];
+ /// ```
+ fn is_nested_delimited_identifier_start(&self, ch: char) -> bool {
+ ch == '['
}
- /// Determine if quoted characters are proper for identifier
- /// It's needed to distinguish treating square brackets as quotes from
- /// treating them as json path. If there is identifier then we assume
- /// there is no json path.
- fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ /// Only applicable whenever [`Self::is_nested_delimited_identifier_start`] returns true
+ /// If the next sequence of tokens potentially represent a nested identifier, then this method
+ /// returns a tuple containing the outer quote style, and if present, the inner (nested) quote style.
+ ///
+ /// Example (Redshift):
+ /// ```text
+ /// `["foo"]` => (Some(`[`), Some(`"`))
+ /// `[foo]` => (Some(`[`), None)
+ /// `"foo"` => None
Review Comment:
Nice catch.
Updated and I added one more example `[0]`
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1872019394
##########
src/dialect/redshift.rs:
##########
@@ -41,14 +41,48 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
Review Comment:
@iffyio I have updated the PR based on your suggestion. Now, a common quoted identifier is processed without additional conditions, and RedShift uses the same set of starting characters for quoted identifiers as PostgreSQL.
Additionally, I added a specific match to handle the RedShift-specific case for [. All tests passed without requiring any 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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1867993041
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
This method is called by [tokeniser](https://github.com/apache/datafusion-sqlparser-rs/blob/main/src/tokenizer.rs#L1078):
```
quote_start
if self.dialect.is_delimited_identifier_start(ch)
&& self
.dialect
.is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
```
It applies to both delimiters of identifiers (`"` and `[`) to determine whether the current character sequence marks the beginning of a quoted token. This code is executed regardless of the specific characters—be it `[0]` or `"a"`.
However, only `[` is considered the start of a quoted identifier in json path case. This PR clarifies this behavior with a more explicit condition at the start of the method.
The highlighted line can only be triggered for `[0]` and never for `"a"`. However, there is a possibility that a double(by number of different type of symbols)-quoted identifier like `["a"]` could be parsed. An additional check for `"` addresses this scenario. Note that JSON path expressions use single quotes `['a']` and never double quotes.
I added unit-tests to cover all these cases.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "bombsimon (via GitHub)" <gi...@apache.org>.
bombsimon commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870388122
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,24 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as
+ // a start character and a quote is a beginning of quoted identifier.
+ // Skipping analyzing token such as `"a"` and analyze only token that
+ // can be part of json path potentially.
+ // For ex., `[0]`, `['a']` (seems part of json path) or `["a"]` (normal quoted identifier)
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
Review Comment:
On `main`, an expression like `SELECT foo["bar"]` evaluates to a `JsonAccess` with the path containing an ident named `bar` but I guess that's wrong. What's a bit interesting though is that with this change, it will evaluate to an expression with alias (`SELECT foo AS ["bar"]`). I know this library doesn't care about what's actually valid SQL, but I wonder if it's an unwanted side effect? I guess it just drops the `[]` and parses `SELECT foo "bar"` which is short for `SELECT foo AS "bar"`.
Another thing, is it intended to end up with `[]` as the quoting style and the value containing the quotes (`"a"`)? Right now `["a"]` evaluates to an ident which I guess is what's causing above. Is `"a"` different from `a` as an identifier?
```rust
Ident {
value: "\"1\"",
quote_style: Some(
'[',
),
},
```
If we encounter a `"` as the first character in `[]` quotes, should we somehow verify what's quoted or ensure we have a closing `"`? If we don't, anything will go, e.g. `["=[]` would parse to an `Ident` with value `"[=` but maybe that's valid PartiQL?
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870464718
##########
src/tokenizer.rs:
##########
@@ -1081,19 +1081,34 @@ impl<'a> Tokenizer<'a> {
.dialect
.is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
- let quote_end = Word::matching_end_quote(quote_start);
- let (s, last_char) = self.parse_quoted_ident(chars, quote_end);
+ let word = if let Some(nested_quote_start) = self
+ .dialect
+ .nested_quote_start(quote_start, chars.peekable.clone())
+ {
+ chars.next(); // consume the opening quote
Review Comment:
This code is RedShift specific (nested quotation). The same time tokenisation of quoted identifier is a method of tokeniser and this is a reason of implemented it here.
Let me know if you have a good idea how makes this code more explicit.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873688579
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +380,23 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ // An alias starting with a number
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1abc" FROM a"#);
+}
+
+#[test]
+fn test_parse_nested_quoted_identifier() {
+ redshift().verified_stmt(r#"SELECT 1 AS ["1"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS [ " 1 " ]"#);
Review Comment:
Ah I think the expected behavior would be that the parser strips out whitespace:
```rust
redshift().one_statement_parses_to(
r#"SELECT 1 AS [ " 1 " ]"#,
r#"SELECT 1 AS [" 1 "]"#,
);
```
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "bombsimon (via GitHub)" <gi...@apache.org>.
bombsimon commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1867939109
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
Gotcha! Maybe that's what confused me because it used to always consume the first token (a `[`?) but now the first token can be a `"` it seems like so I guess we reach this code even with `"A"` as in the PR description?
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1871749957
##########
src/dialect/redshift.rs:
##########
@@ -41,14 +41,48 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
Review Comment:
Wondering why redshift is special with this, Im thinking since there are other dialects that support jsonpath (which implements partiql in the parser) it shouldn't be. Would it not be enough to remove this `is_proper_identifier_inside_quotes` method entirely and the special `[` character from `is_delimited_identifier_start`?
From what I can tell in [their docs](https://docs.aws.amazon.com/redshift/latest/dg/r_names.html), `[` isn't an identifier delimiter so it seems that having the dialect `is_delimited_identifier_start` flag otherwise is incorrect?
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870413689
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,24 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as
+ // a start character and a quote is a beginning of quoted identifier.
+ // Skipping analyzing token such as `"a"` and analyze only token that
+ // can be part of json path potentially.
+ // For ex., `[0]`, `['a']` (seems part of json path) or `["a"]` (normal quoted identifier)
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
Review Comment:
Thank you for feedback!
A goal of this MR is improving implementation of RedShift dialect. The method `is_proper_identifier_inside_quotes` is implemented only for RedShift and I assume that it added only for RedShift specific case.
I verified one more time and found that `SELECT foo["bar"] FROM table_name` or `SELECT foo['bar'] FROM table_name` is not valid in RedShift case. `SELECT foo."bar" FROM table_name` is only valid for this certain case. We don't need to care about this case. I updated comments.
I'm going to improve verification of quoted identifier in this special case that you mentioned. Thank you for highlighting this edge case!
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873382034
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +378,20 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ // RedShift specific case - quoted identifier inside square bracket
Review Comment:
I split the test into two to make it more explicit:
1. `test_parse_select_numbered_columns` - it is a reason for this MR
2. `test_parse_nested_quoted_identifier`
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1871857040
##########
src/dialect/redshift.rs:
##########
@@ -41,14 +41,48 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
Review Comment:
It is a reasonable question!
I verified what happened if `is_proper_identifier_inside_quotes` is removed and `[` is removed from RedShift dialect - a couple tests failed `brackets_over_db_schema_table_name_with_whites_paces` and `test_square_brackets_over_db_schema_table_name` + my new one. Both are related to an old fashion way of using `[` in RedShift.
I verified that these queries work in RedShift and we can find it in query history or in end-user usages:
1. `SELECT 1 AS ["1"] FROM foo`
2. `SELECT [a] FROM [ schema ].[foo]`
Perhaps, needs to add a special case for `[` in RedShift dialect instead of improving identifying a possible identifier inside quotes.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1870465243
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,24 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as
+ // a start character and a quote is a beginning of quoted identifier.
+ // Skipping analyzing token such as `"a"` and analyze only token that
+ // can be part of json path potentially.
+ // For ex., `[0]`, `['a']` (seems part of json path) or `["a"]` (normal quoted identifier)
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
Review Comment:
I added more unit-tests and updated a code of tokeniser to handle mentioned edge cases.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873380142
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,23 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// RedShift support old way of quotation with `[` and it can cover even nested quoted identifier.
+ fn is_nested_delimited_identifier_start(&self, _ch: char) -> bool {
+ false
+ }
+
+ /// Determine if nested quoted characters are presented
+ fn nested_delimited_identifier(
Review Comment:
Renamed
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
Review Comment:
Updated
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
quote_start
- if self.dialect.is_delimited_identifier_start(ch)
+ if self
+ .dialect
+ .is_nested_delimited_identifier_start(quote_start)
&& self
.dialect
- .is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
+ .nested_delimited_identifier(chars.peekable.clone())
+ .is_some() =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
+ let (quote_start, nested_quote_start) = self
+ .dialect
+ .nested_delimited_identifier(chars.peekable.clone())
+ .unwrap();
Review Comment:
Fixed.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873381493
##########
tests/sqlparser_redshift.rs:
##########
@@ -279,6 +279,31 @@ fn test_redshift_json_path() {
},
expr_from_projection(only(&select.projection))
);
+
+ let sql = r#"SELECT db1.sc1.tbl1.col1[0]."id" FROM customer_orders_lineitem"#;
+ let select = dialects.verified_only_select(sql);
+ assert_eq!(
+ &Expr::JsonAccess {
+ value: Box::new(Expr::CompoundIdentifier(vec![
+ Ident::new("db1"),
+ Ident::new("sc1"),
+ Ident::new("tbl1"),
+ Ident::new("col1")
+ ])),
+ path: JsonPath {
+ path: vec![
+ JsonPathElem::Bracket {
+ key: Expr::Value(Value::Number("0".parse().unwrap(), false))
Review Comment:
All similar lines in `sqlparser_redshift.rs` are updated.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio merged PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1867798790
##########
src/dialect/redshift.rs:
##########
@@ -41,10 +41,19 @@ impl Dialect for RedshiftSqlDialect {
/// treating them as json path. If there is identifier then we assume
/// there is no json path.
fn is_proper_identifier_inside_quotes(&self, mut chars: Peekable<Chars<'_>>) -> bool {
+ // PartiQL (used as json path query language in Redshift) uses square bracket as a start character and a quote is a beginning of quoted identifier
+ if let Some(quote_start) = chars.peek() {
+ if *quote_start == '"' {
+ return true;
+ }
+ };
chars.next();
let mut not_white_chars = chars.skip_while(|ch| ch.is_whitespace()).peekable();
if let Some(&ch) = not_white_chars.peek() {
- return self.is_identifier_start(ch);
+ // PartiQL uses single quote as starting identification inside a quote
+ // It is a normal identifier if it has no single quote at the beginning.
+ // Additionally square bracket can contain quoted identifier.
+ return ch == '"' || ch != '\'' && self.is_identifier_start(ch);
Review Comment:
Good point! I've removed the extra check.
The expression can be `foo.field[0]['ab']`, but `foo.field[0]["ab"]` is invalid (I verified this on Redshift).
An identifier like `"'foo'"` never reaches this point. A quoted identifier starting with `"` is already checked in the condition above.
This method is designed to handle special cases of JSON paths, and it expects the input to start with a square bracket. I'm working to clarify this in this MR.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873272299
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
quote_start
- if self.dialect.is_delimited_identifier_start(ch)
+ if self
+ .dialect
+ .is_nested_delimited_identifier_start(quote_start)
&& self
.dialect
- .is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
+ .nested_delimited_identifier(chars.peekable.clone())
+ .is_some() =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
+ let (quote_start, nested_quote_start) = self
+ .dialect
+ .nested_delimited_identifier(chars.peekable.clone())
+ .unwrap();
Review Comment:
```suggestion
```
we can return an error here?
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,23 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// RedShift support old way of quotation with `[` and it can cover even nested quoted identifier.
Review Comment:
```suggestion
/// Example: RedShift supports the following quote styles to all mean the same thing:
/// ```sql
/// SELECT 1 AS foo;
/// SELECT 1 AS "foo";
/// SELECT 1 AS [foo];
/// SELECT 1 AS ["foo"];
/// ```
```
Thinking something like this to clarify the behavior?
##########
tests/sqlparser_redshift.rs:
##########
@@ -279,6 +279,31 @@ fn test_redshift_json_path() {
},
expr_from_projection(only(&select.projection))
);
+
+ let sql = r#"SELECT db1.sc1.tbl1.col1[0]."id" FROM customer_orders_lineitem"#;
+ let select = dialects.verified_only_select(sql);
+ assert_eq!(
+ &Expr::JsonAccess {
+ value: Box::new(Expr::CompoundIdentifier(vec![
+ Ident::new("db1"),
+ Ident::new("sc1"),
+ Ident::new("tbl1"),
+ Ident::new("col1")
+ ])),
+ path: JsonPath {
+ path: vec![
+ JsonPathElem::Bracket {
+ key: Expr::Value(Value::Number("0".parse().unwrap(), false))
Review Comment:
```suggestion
key: Expr::Value(number("0"))
```
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
quote_start
- if self.dialect.is_delimited_identifier_start(ch)
+ if self
+ .dialect
+ .is_nested_delimited_identifier_start(quote_start)
&& self
.dialect
- .is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
+ .nested_delimited_identifier(chars.peekable.clone())
+ .is_some() =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
+ let (quote_start, nested_quote_start) = self
+ .dialect
+ .nested_delimited_identifier(chars.peekable.clone())
+ .unwrap();
+
+ let Some(nested_quote_start) = nested_quote_start else {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ return Ok(Some(Token::make_word(&word, Some(quote_start))));
+ };
+
+ let mut word = vec![];
let quote_end = Word::matching_end_quote(quote_start);
- let (s, last_char) = self.parse_quoted_ident(chars, quote_end);
+ let nested_quote_end = Word::matching_end_quote(nested_quote_start);
+ let error_loc = chars.location();
- if last_char == Some(quote_end) {
- Ok(Some(Token::make_word(&s, Some(quote_start))))
- } else {
- self.tokenizer_error(
+ chars.next(); // skip the first delimiter
+ word.push(peeking_take_while(chars, |ch| ch.is_whitespace()));
+ if chars.peek() != Some(&nested_quote_start) {
+ return self.tokenizer_error(
+ error_loc,
+ format!("Expected nested delimiter '{nested_quote_start}' before EOF."),
+ );
+ }
+ word.push(format!("{nested_quote_start}"));
Review Comment:
```suggestion
word.push(nested_quote_start.into()));
```
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,23 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// RedShift support old way of quotation with `[` and it can cover even nested quoted identifier.
+ fn is_nested_delimited_identifier_start(&self, _ch: char) -> bool {
+ false
+ }
+
+ /// Determine if nested quoted characters are presented
+ fn nested_delimited_identifier(
Review Comment:
```suggestion
fn peek_nested_delimited_identifier_quotes(
```
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
Review Comment:
```suggestion
// Potentially nested delimited (quoted) identifier
```
##########
src/dialect/redshift.rs:
##########
@@ -32,21 +32,36 @@ pub struct RedshiftSqlDialect {}
// in the Postgres dialect, the query will be parsed as an array, while in the Redshift dialect it will
// be a json path
impl Dialect for RedshiftSqlDialect {
- fn is_delimited_identifier_start(&self, ch: char) -> bool {
- ch == '"' || ch == '['
+ fn is_nested_delimited_identifier_start(&self, ch: char) -> bool {
+ ch == '['
}
- /// Determine if quoted characters are proper for identifier
+ /// Determine if quoted characters are looks like special case of quotation begining with `[`.
Review Comment:
Maybe we can copy over the suggested dialect method comment here since its already describing redshift.
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +378,20 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
Review Comment:
thinking a couple cases we can include?
```sql
SELECT 1 AS ["1];
SELECT 1 AS [ " 1 " ];
```
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,23 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// RedShift support old way of quotation with `[` and it can cover even nested quoted identifier.
+ fn is_nested_delimited_identifier_start(&self, _ch: char) -> bool {
+ false
+ }
+
+ /// Determine if nested quoted characters are presented
Review Comment:
```suggestion
/// Only applicable whenever [`Self::is_nested_delimited_identifier_start`] returns true
/// If the next sequence of tokens potentially represent a nested identifier, then this method
/// returns a tuple containing the outer quote style, and if present, the inner (nested) quote style.
///
/// Example (Redshift):
/// ```text
/// `["foo"]` => (Some(`[`), Some(`"`))
/// `[foo]` => (Some(`[`), None)
/// `"foo"` => None
/// ```
```
Similarly, to clarify the behavior with an example since it wouldn't be intuitive otherwise
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +378,20 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ // RedShift specific case - quoted identifier inside square bracket
Review Comment:
```suggestion
```
this one seems to be already covered by `test_parse_create_numbered_columns`?
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
quote_start
- if self.dialect.is_delimited_identifier_start(ch)
+ if self
+ .dialect
+ .is_nested_delimited_identifier_start(quote_start)
&& self
.dialect
- .is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
+ .nested_delimited_identifier(chars.peekable.clone())
+ .is_some() =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
+ let (quote_start, nested_quote_start) = self
+ .dialect
+ .nested_delimited_identifier(chars.peekable.clone())
+ .unwrap();
+
+ let Some(nested_quote_start) = nested_quote_start else {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ return Ok(Some(Token::make_word(&word, Some(quote_start))));
+ };
+
+ let mut word = vec![];
let quote_end = Word::matching_end_quote(quote_start);
- let (s, last_char) = self.parse_quoted_ident(chars, quote_end);
+ let nested_quote_end = Word::matching_end_quote(nested_quote_start);
+ let error_loc = chars.location();
- if last_char == Some(quote_end) {
- Ok(Some(Token::make_word(&s, Some(quote_start))))
- } else {
- self.tokenizer_error(
+ chars.next(); // skip the first delimiter
+ word.push(peeking_take_while(chars, |ch| ch.is_whitespace()));
+ if chars.peek() != Some(&nested_quote_start) {
+ return self.tokenizer_error(
+ error_loc,
+ format!("Expected nested delimiter '{nested_quote_start}' before EOF."),
+ );
+ }
+ word.push(format!("{nested_quote_start}"));
+ word.push(self.tokenize_quoted_identifier(nested_quote_end, chars)?);
+ word.push(format!("{nested_quote_end}"));
Review Comment:
```suggestion
word.push(nested_quote_end.into());
```
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +378,20 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ // RedShift specific case - quoted identifier inside square bracket
+ redshift().verified_stmt(r#"SELECT 1 AS ["1"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS ["[="] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS ["=]"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS ["a[b]"] FROM a"#);
+}
+
+#[test]
+fn test_parse_create_numbered_columns() {
+ redshift_and_generic().verified_stmt(
+ r#"CREATE TABLE test_table_1 ("1" INT, "d" VARCHAR(155), "2" DOUBLE PRECISION)"#,
+ );
+}
Review Comment:
Can we incorporate this as a test case under the `parse_delimited_identifiers` test?
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873379618
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,23 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// RedShift support old way of quotation with `[` and it can cover even nested quoted identifier.
+ fn is_nested_delimited_identifier_start(&self, _ch: char) -> bool {
+ false
+ }
+
+ /// Determine if nested quoted characters are presented
Review Comment:
Updated.
##########
src/dialect/mod.rs:
##########
@@ -128,14 +128,23 @@ pub trait Dialect: Debug + Any {
ch == '"' || ch == '`'
}
- /// Return the character used to quote identifiers.
- fn identifier_quote_style(&self, _identifier: &str) -> Option<char> {
+ /// Determine if a character starts a potential nested quoted identifier.
+ /// RedShift support old way of quotation with `[` and it can cover even nested quoted identifier.
Review Comment:
Updated.
##########
src/dialect/redshift.rs:
##########
@@ -32,21 +32,36 @@ pub struct RedshiftSqlDialect {}
// in the Postgres dialect, the query will be parsed as an array, while in the Redshift dialect it will
// be a json path
impl Dialect for RedshiftSqlDialect {
- fn is_delimited_identifier_start(&self, ch: char) -> bool {
- ch == '"' || ch == '['
+ fn is_nested_delimited_identifier_start(&self, ch: char) -> bool {
+ ch == '['
}
- /// Determine if quoted characters are proper for identifier
+ /// Determine if quoted characters are looks like special case of quotation begining with `[`.
Review Comment:
Comments copied.
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873380547
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +378,20 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ // RedShift specific case - quoted identifier inside square bracket
+ redshift().verified_stmt(r#"SELECT 1 AS ["1"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS ["[="] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS ["=]"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS ["a[b]"] FROM a"#);
+}
+
+#[test]
+fn test_parse_create_numbered_columns() {
+ redshift_and_generic().verified_stmt(
+ r#"CREATE TABLE test_table_1 ("1" INT, "d" VARCHAR(155), "2" DOUBLE PRECISION)"#,
+ );
+}
Review Comment:
I moved this special case (numbered named alias) into the `parse_delimited_identifiers` test.
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +378,20 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
Review Comment:
I split the test into two to make it more explicit:
1. `test_parse_select_numbered_columns` - it is a reason for this MR
2. `test_parse_nested_quoted_identifier`
I added suggested cases into `test_parse_nested_quoted_identifier`
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "7phs (via GitHub)" <gi...@apache.org>.
7phs commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1873381184
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
quote_start
- if self.dialect.is_delimited_identifier_start(ch)
+ if self
+ .dialect
+ .is_nested_delimited_identifier_start(quote_start)
&& self
.dialect
- .is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
+ .nested_delimited_identifier(chars.peekable.clone())
+ .is_some() =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
+ let (quote_start, nested_quote_start) = self
+ .dialect
+ .nested_delimited_identifier(chars.peekable.clone())
+ .unwrap();
+
+ let Some(nested_quote_start) = nested_quote_start else {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ return Ok(Some(Token::make_word(&word, Some(quote_start))));
+ };
+
+ let mut word = vec![];
let quote_end = Word::matching_end_quote(quote_start);
- let (s, last_char) = self.parse_quoted_ident(chars, quote_end);
+ let nested_quote_end = Word::matching_end_quote(nested_quote_start);
+ let error_loc = chars.location();
- if last_char == Some(quote_end) {
- Ok(Some(Token::make_word(&s, Some(quote_start))))
- } else {
- self.tokenizer_error(
+ chars.next(); // skip the first delimiter
+ word.push(peeking_take_while(chars, |ch| ch.is_whitespace()));
+ if chars.peek() != Some(&nested_quote_start) {
+ return self.tokenizer_error(
+ error_loc,
+ format!("Expected nested delimiter '{nested_quote_start}' before EOF."),
+ );
+ }
+ word.push(format!("{nested_quote_start}"));
Review Comment:
Updated
##########
src/tokenizer.rs:
##########
@@ -1075,25 +1075,56 @@ impl<'a> Tokenizer<'a> {
Ok(Some(Token::DoubleQuotedString(s)))
}
// delimited (quoted) identifier
+ quote_start if self.dialect.is_delimited_identifier_start(ch) => {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ Ok(Some(Token::make_word(&word, Some(quote_start))))
+ }
+ // special (quoted) identifier
quote_start
- if self.dialect.is_delimited_identifier_start(ch)
+ if self
+ .dialect
+ .is_nested_delimited_identifier_start(quote_start)
&& self
.dialect
- .is_proper_identifier_inside_quotes(chars.peekable.clone()) =>
+ .nested_delimited_identifier(chars.peekable.clone())
+ .is_some() =>
{
- let error_loc = chars.location();
- chars.next(); // consume the opening quote
+ let (quote_start, nested_quote_start) = self
+ .dialect
+ .nested_delimited_identifier(chars.peekable.clone())
+ .unwrap();
+
+ let Some(nested_quote_start) = nested_quote_start else {
+ let word = self.tokenize_quoted_identifier(quote_start, chars)?;
+ return Ok(Some(Token::make_word(&word, Some(quote_start))));
+ };
+
+ let mut word = vec![];
let quote_end = Word::matching_end_quote(quote_start);
- let (s, last_char) = self.parse_quoted_ident(chars, quote_end);
+ let nested_quote_end = Word::matching_end_quote(nested_quote_start);
+ let error_loc = chars.location();
- if last_char == Some(quote_end) {
- Ok(Some(Token::make_word(&s, Some(quote_start))))
- } else {
- self.tokenizer_error(
+ chars.next(); // skip the first delimiter
+ word.push(peeking_take_while(chars, |ch| ch.is_whitespace()));
+ if chars.peek() != Some(&nested_quote_start) {
+ return self.tokenizer_error(
+ error_loc,
+ format!("Expected nested delimiter '{nested_quote_start}' before EOF."),
+ );
+ }
+ word.push(format!("{nested_quote_start}"));
+ word.push(self.tokenize_quoted_identifier(nested_quote_end, chars)?);
+ word.push(format!("{nested_quote_end}"));
Review Comment:
Updated
--
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] Redshift: Fix parsing for quoted numbered columns [datafusion-sqlparser-rs]
Posted by "iffyio (via GitHub)" <gi...@apache.org>.
iffyio commented on code in PR #1576:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1576#discussion_r1874372151
##########
tests/sqlparser_redshift.rs:
##########
@@ -353,3 +380,23 @@ fn test_parse_json_path_from() {
_ => panic!(),
}
}
+
+#[test]
+fn test_parse_select_numbered_columns() {
+ // An alias starting with a number
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1" FROM a"#);
+ redshift_and_generic().verified_stmt(r#"SELECT 1 AS "1abc" FROM a"#);
+}
+
+#[test]
+fn test_parse_nested_quoted_identifier() {
+ redshift().verified_stmt(r#"SELECT 1 AS ["1"] FROM a"#);
+ redshift().verified_stmt(r#"SELECT 1 AS [ " 1 " ]"#);
Review Comment:
Oh yeah I think that the linked test would be incorrect as well? from what I could tell and tested the following seemed to all be equivalent in redshift
```sql
SELECT 1 AS foo;
SELECT 1 AS [ foo ];
SELECT 1 AS [ "foo" ];
```
so that whitespace within the brackets isn't part of the identifier. Then it would be incorrect to have an identifier in the AST that contains the string ` foo ` for any of the above.
On the other hand this would actually be referencing an identifier containing whitespace
```sql
SELECT 1 AS [" foo "];
```
--
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