Fix: parsing ident starting with underscore in certain dialects#1835
Fix: parsing ident starting with underscore in certain dialects#1835iffyio merged 4 commits intoapache:mainfrom
Conversation
src/tokenizer.rs
Outdated
| Some('_') => { | ||
| self.tokenizer_error( | ||
| chars.location(), | ||
| "Unexpected underscore here".to_string(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
I wonder if its worth returning an error here or whether we lose anything by allowing the tokenizer continue? I'm guessing its still possible for the tokenizer to return Token::Period here as well?
There was a problem hiding this comment.
AFAIK underscore after a dot can only be an identifier, so it makes sense to error here
There was a problem hiding this comment.
without the errors, '._123' as a whole will be parsed as a number, which is not valid in any standard AFAICT.
'._abc' as a whole would also be parsed as a word, which doesn't make sense because the prev token was not a word.
src/tokenizer.rs
Outdated
| let is_number_separator = |ch: char, next_char: Option<char>| { | ||
| self.dialect.supports_numeric_literal_underscores() | ||
| && ch == '_' | ||
| && next_char.is_some_and(|c| c.is_ascii_digit()) | ||
| }; | ||
|
|
||
| s += &peeking_next_take_while(chars, |ch, next_ch| { | ||
| ch.is_ascii_digit() || is_number_separator(ch, next_ch) | ||
| }); | ||
|
|
||
| // Handle exponent part | ||
| if matches!(chars.peek(), Some('e' | 'E')) { | ||
| let mut exp = String::new(); | ||
| exp.push(chars.next().unwrap()); | ||
|
|
||
| if matches!(chars.peek(), Some('+' | '-')) { | ||
| exp.push(chars.next().unwrap()); | ||
| } | ||
|
|
||
| if matches!(chars.peek(), Some(c) if c.is_ascii_digit()) { | ||
| exp += &peeking_take_while(chars, |c| c.is_ascii_digit()); | ||
| s += &exp; | ||
| } | ||
| } | ||
|
|
||
| // Handle "L" suffix for long numbers | ||
| let long = if chars.peek() == Some(&'L') { | ||
| chars.next(); | ||
| true |
There was a problem hiding this comment.
hmm most of this logic looks to already be duplicated on the number parsing code path, so that that side effect would be undesirable I think.
If I understood the issue being solved for, its only the case of ._ being parsed as a number, would it be possible/more-desirable to only update the existing logic to properly detect and handle that case or is the current logic not well equipped to handle that sanely?
There was a problem hiding this comment.
I know. I tried to fix it without splitting the branches, but I couldn't.
Feel free to take a go at it if possible
There was a problem hiding this comment.
The problem is the match happens on a peek and you need to consume the dot in order to peek the underscore.
What if the second peek wasn't an underscore? You need to un-consume the dot for it to be parsed as part of the number.
There was a problem hiding this comment.
Does the peekable_clone help in this case maybe? in order to lookahead without consuming characters
There was a problem hiding this comment.
Of course, but I prefer not cloning the entire token stream, especially since the dup code is only a few lines.
There was a problem hiding this comment.
Would you prefer pulling the dup code into functions, if possible?
There was a problem hiding this comment.
Oh is that really the case that cloning the peekable iterator would clone the entire stream? (iirc it should only clone a struct with pointer offsets), the functionality is used in a few places in the tokenizer so I think it would be preferrable to avoid dup logic (if it turns out its an expensive thing to do then maybe we need to do a pass through the tokenizer to avoid that pattern overall, but that would be unrelated to this PR ofc)
The dialects that support underscore as a separator in numeric literals used to parse ._123 as a number, meaning that an identifier like ._abc would be parsed as Number `._` and word `abc`, which is obv wrong. This PR splits the tokenizer branch for numbers and periods into two branches to make things easier, fixes the issue mentioned above and adds tests.
iffyio
left a comment
iffyio
left a comment
There was a problem hiding this comment.
LGTM! Thanks @MohamedAbdeen21!
cc @alamb
2 participants
The dialects that support underscore as a separator in numeric literals used to parse
._123as a number, which I don't think is valid SQL. However, that means that something like._abcwould be parsed as Number._and wordabc, which is wrong.This PR splits the tokenizer match branch for numbers and periods into two branches to make things easier, fixes the issue mentioned above, and adds tests.
CC: @mvzink