Skip to content

Enable dialect specific behaviours in the parser #254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

eyalleshem
Copy link
Contributor

@eyalleshem eyalleshem commented Aug 2, 2020

[nickolay: added the following from the final commit message]

  • Change Parser { ... } to store the dialect used:
    Parser<'a> { ... dialect: &'a dyn Dialect }

    Thanks to @c7hm4r for the initial version of this submitted as
    part of Add support for "on delete cascade" column option #170

  • Introduce dialect_of!(parser is SQLiteDialect | GenericDialect) helper
    to branch on the dialect's type

  • Use the new functionality to make AUTO_INCREMENT and AUTOINCREMENT
    parsing dialect-dependent.

@coveralls
Copy link

coveralls commented Aug 2, 2020

Pull Request Test Coverage Report for Build 195813271

  • 25 of 25 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 91.919%

Totals Coverage Status
Change from base Build 195809538: 0.03%
Covered Lines: 4527
Relevant Lines: 4925

💛 - Coveralls

Copy link
Contributor

@nickolay nickolay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

To give some context, this continues from #241 (comment) and combines an earlier draft c67d667 with the ability to branch on the dialect identity. It was split from #244.

Comment on lines 44 to 45
/// The name of the dialect
fn dialect_name(&self) -> &'static str;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of using strings for checking for a particular dialect in the parser code, especially because a typo in the dialect name (as the one in this PR) can easily go unnoticed. The best I can come up with is:

pub trait Dialect: Debug + Any {
...
impl dyn Dialect {
    #[inline]
    pub fn is<T: Dialect>(&self) -> bool { // borrowed from `Any` implementation
        TypeId::of::<T>() == self.type_id()
    }
}

...
if self.dialect.is::<MsSqlDialect>() {

@Dandandan or @maxcountryman do you have any thoughts on this?

Copy link
Contributor Author

@eyalleshem eyalleshem Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep- the static type is much better , i wasn't aware of the ANY trait in RUST .
I tried to take the same approach but with macro's WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since GitHub thinks this branch of discussion is "outdated', I continued below.

@eyalleshem eyalleshem force-pushed the dialect_specfic_behaviour_in_the_parser branch from 1a56d86 to e8b1dd5 Compare August 3, 2020 07:00
@eyalleshem eyalleshem requested a review from nickolay August 3, 2020 07:17
@eyalleshem eyalleshem force-pushed the dialect_specfic_behaviour_in_the_parser branch from e8b1dd5 to 742c50e Compare August 3, 2020 07:39
@nickolay
Copy link
Contributor

nickolay commented Aug 3, 2020

Continuing from above

Yep- the static type is much better , i wasn't aware of the ANY trait in RUST .
I tried to take the same approach but with macro's WDYT ?

I thought macros could be layered on top of Dialect's pub fn is<T: Dialect>(&self) -> bool. Why do you prefer to make it accept any type ids by changing it to fn is_dialect(&self, type_id: TypeId) -> bool? It makes the intent less clear to me and invites the question of why such a generic function has to be defined on Dialect (IIRC, it has to, but it's not obvious that it does).

If we use a macro for branching, we could make it accept a list of dialects, e.g. dialect_of!(self is MySqlDialect | GenericDialect) with

macro_rules! dialect_of {
    { $parsed_dialect: ident is $($dialect_type: ty)|+ } => {
        ($($parsed_dialect.dialect.is::<$dialect_type>())||+)
    };
}

@eyalleshem eyalleshem force-pushed the dialect_specfic_behaviour_in_the_parser branch from 742c50e to cae5d37 Compare August 4, 2020 06:57
@eyalleshem
Copy link
Contributor Author

Ok , updated the commit according to the suggestion .

Copy link
Contributor

@nickolay nickolay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ready to merge this, but I'll give the rest of the team a few days to react. I don't think your other PRs have to wait for this one to be merged?

@andygrove thought I ought to mention you explicitly here, given your previous interest in this topic. Although I understand you don't have much time for this these days. (If you're interested in diving in, the context is in my previous comment.)

Comment on lines 31 to 32
/// Determine if the specified dialect matched to
/// the parsed dialect , used for dialect spefic behaviour
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an example will be easier to understand here:

/// `dialect_of!(parser is SQLiteDialect |  GenericDialect)` evaluates
/// to `true` iff `parser.dialect` is one of the `Dialect`s specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@eyalleshem eyalleshem force-pushed the dialect_specfic_behaviour_in_the_parser branch from cae5d37 to 1bed7b0 Compare August 4, 2020 15:06
@eyalleshem eyalleshem force-pushed the dialect_specfic_behaviour_in_the_parser branch from 1bed7b0 to c87f916 Compare August 5, 2020 06:15
@nickolay
Copy link
Contributor

nickolay commented Aug 9, 2020

Hey @eyalleshem, sorry for the delay. I wanted to merge this, but found that you added unrelated functionality in the most recent force-push. Could you reset this PR back to 1bed7b0 ? It's not available to me or my git-fu is too weak for this.

@eyalleshem
Copy link
Contributor Author

Hi @nickolay, by mistake i added some functionally to this PR before 6 days , but it's removed immediately - the current content of the current commit in the PR , is same as 1bed7b0, I think the commit hash's are different because the PR rebased On the main branch , while 1bed7b0 not . sorry for the mess..

@nickolay nickolay merged commit 1b46e82 into apache:main Aug 10, 2020
@nickolay
Copy link
Contributor

You're right, I got confused. Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants