Extended dialect triat to support numeric prefixed identifiers #1188
Conversation
Pull Request Test Coverage Report for Build 8943194087Details
💛 - Coveralls |
There was a problem hiding this comment.
Thank you for this contribution @groobyming -- I have a suggestion that I think would make the code cleaner and perhaps also work more like how you want
I apologize for the delay in reviewing
src/tokenizer.rs
Outdated
| @@ -800,7 +800,9 @@ impl<'a> Tokenizer<'a> { | |||
|
|
|||
| // mysql dialect supports identifiers that start with a numeric prefix, | |||
| // as long as they aren't an exponent number. | |||
| if dialect_of!(self is MySqlDialect | HiveDialect) && exponent_part.is_empty() { | |||
| if (dialect_of!(self is MySqlDialect | HiveDialect) && exponent_part.is_empty()) | |||
| || self.dialect.supports_numeric_prefix() | |||
There was a problem hiding this comment.
I wonder why the check for self.dialect.supports_numeric_prefix() doesn't also check exponent_part.is_empty?
That would mean that tokens like 1e4 are going to be parsed as identifiers
What if you wrote this check like
if self.dialect.supports_numeric_prefix() && exponent_part.is_empty()) {And then changed MySqlDialect and HiveDialect to return true for supports_numeric_prefix?
There was a problem hiding this comment.
Great suggestion! I overlooked this scenario, and I'll fix this logic issue
src/dialect/mod.rs
Outdated
| @@ -143,6 +143,10 @@ pub trait Dialect: Debug + Any { | |||
| fn supports_start_transaction_modifier(&self) -> bool { | |||
| false | |||
| } | |||
| /// Returns true if the dialect supports identifiers that start with a numeric prefix | |||
There was a problem hiding this comment.
Could you please add an example of such an identifier? (e.g. 123Column)?
There was a problem hiding this comment.
Our scenario will use numbers as business ids. The rule for table names is the business id plus an underscore followed by the specific table name.
eg：59901_user_login
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
groobyming
force-pushed
the
add_support_numeri_prefix
branch
from
20e3ad5 to
b7f0d18
Compare
There was a problem hiding this comment.
Thanks again @groobyming -- I took the liberty of moving the check to be entirely trait based and merging up to resolve conflicts.
…e#1188) Co-authored-by: Andrew Lamb <[email protected]>
Relates to #1186. This PR includes changes to resolve the issue and enhance project functionality. Review appreciated.