Skip to content
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

[Discussion] Remove use of dialect_of macro #339

Open
ever0de opened this issue Aug 25, 2021 · 8 comments
Open

[Discussion] Remove use of dialect_of macro #339

ever0de opened this issue Aug 25, 2021 · 8 comments

Comments

@ever0de
Copy link
Contributor

ever0de commented Aug 25, 2021

What was the purpose of the current Dialect?
The dialect_of macro makes it difficult to modify your code whenever you add support for Dialect.
And besides the identifier support of Dialect, it seems that it can also support query (Expr).

But... if you have other intentions for macros
Since there is a problem, I would like to discuss it with the maintainers.

@ever0de
Copy link
Contributor Author

ever0de commented Aug 25, 2021

  • When implementing the TRIM function function
  • Different Dialects support standard and non-standard.

E.g

  • in Dialect Trait
    fn is_delimited_identifier_start(&self, ch: char);
    fn is_identifier_start(&self, ch: char) -> bool;
    fn is_identifier_part(&self, ch: char) -> bool;
    ...

    fn is_standard_trim_supported(..) -> bool {
        // e.g. TRIM(LEADING FROM "ABC")
        true
    }

    fn is_non_standard_trim_supported(..) -> bool {
        // e.g. TRIM("ABC", "A");
        true
    }
}

PostgreSQL

image

MySQL

image

Sqlite

image

@ever0de
Copy link
Contributor Author

ever0de commented Aug 25, 2021

There was an issue related to this

After some more thought, how about managing Dialect with features?

@ever0de
Copy link
Contributor Author

ever0de commented Aug 25, 2021

Ast Display should also be considered.
Looks like this should be a different issue.

@nickolay
Copy link
Contributor

The dialect_of!(parser is Dialect1 | Dialect2 | GenericDialect) helper was introduced in #254 to let the parser branch on the dialect's type. An example where such branching is required is when the grammars of various dialects conflict, e.g. #241 (comment)

I do prefer branching on specific dialects in the parser to adding flags (like is_standard_trim_supported / is_non_standard_trim_supported in your example) to the dialect trait and I do not understand what problem you are having with dialect_of!.

@AugustoFKL
Copy link
Contributor

@nickolay can't we have a default implementation for stuff that is generic, i.e., is specified on the ANSII, and specific implementation overwriting them if the child ever needs it?

E.g.,

impl GenericDialect {
   fn parse_limit() {
        #fail
   }
}

impl MySqlDialect {
    fn parse_limit() {
        #specific implementation
    }
}

@nickolay
Copy link
Contributor

nickolay commented Oct 1, 2022

You could technically do that, but why?

Current design aims to produce a parser that’s able to parse SQL from common dialects with minimal maintenance effort. Producing “strict” dialect-specific parsers, that reject syntax not supported by the given dialect, would require maintaining a separate parser for each dialect — so it isn’t a goal of this project. One can fork and remove the unneeded code if they need a strict parser.

Note that the “generic” dialect accepts as much dialect specific stuff as possible; not the “lowest common denominator” dialect, and not the ANSI dialect, as you think. So with the approach you suggested, we’d have two copies of all dialect-specific code: one in the dialect-specific trait and another in the generic trait.

With that in mind, and given that all trait implementations must return the same AST type, in most cases branching on the dialect in the shared parser code just seems to make more sense.

@ever0de
Copy link
Contributor Author

ever0de commented Oct 1, 2022

So that's not the goal of this project.

I understood this answer.
Please close the issue when the discussion is over, Thanks!

@AugustoFKL
Copy link
Contributor

@nickolay makes sense, 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

No branches or pull requests

3 participants