Fix redshift specific character #475
Conversation
Pull Request Test Coverage Report for Build 2233206158Warning: 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 |
|
@alamb appreciate your feedback on this one, as it's more complicated then other PRs |
| // We don't yet support identifiers beginning with "letters with | ||
| // diacritical marks and non-Latin letters" | ||
| ('a'..='z').contains(&ch) || ('A'..='Z').contains(&ch) || ch == '_' | ||
| ('a'..='z').contains(&ch) || ('A'..='Z').contains(&ch) || ch == '_' || ch == '#' |
There was a problem hiding this comment.
Yeah I am torn on this one - I worry about causing trouble for someone who is relaying on # not being a valid pg identifiers. I don't really have a great understanding of what people are doing / using sqlparser for and I would say it is not particularly consistent about disallowing constructs that are not valid other dialects of SQL 🤔
What do you think about adding a RedshiftSqlDialect to avoid unintended consquences? I think it could be pretty straightforward, something like:
impl Dialect for RedshiftSqlDialect {
fn is_identifier_start(&self, ch: char) -> bool {
PostgreSqlDialect::is_identifier_start(ch) || ch == '#'
}
}
| return Ok(Some(Token::Number(s, false))); | ||
| } | ||
| Ok(Some(Token::make_word(&s, None))) | ||
| if s == "#" { |
There was a problem hiding this comment.
@alamb What do you think about this change?
I'm worried about clash with lines 610:
I didn't found a better way to solve it.
'#' if dialect_of!(self is SnowflakeDialect) => { chars.next(); // consume the '#', starting a snowflake single-line comment let comment = self.tokenize_single_line_comment(chars); Ok(Some(Token::Whitespace(Whitespace::SingleLineComment { prefix: "#".to_owned(), comment, }))) }
There was a problem hiding this comment.
Maybe we should check if the dialect is RedshiftSqlDialect?
There was a problem hiding this comment.
The change doesn't feel right to me (because it effectively skips #s so wouldn't it mean that things like #SELECT will parse as SELECT (not just identifiers)?
|
I just noticed an outstanding PR from @mskrzypkows that adds a |
I totally agree. I think that I will wait for #471 to be merged, and then I will try to make this PR to work only on Redshift. |
3 participants
The following query is valid in Redshift:
In postgres, when running using pgadmin, the query is not valid and must written as the following:
(I'm not sure what is the behavior in other drivers/apps that can query postgres).
As redshift is based on postgres, for me it makes sense to change the postgres dialect.
I would love for some feedback.
Thanks!