Should use the dialect guard by default while supporting new syntaxes

[git-hulk]

Currently, seems we doesn't have a rule whether to have the dialect guard or not. I saw some syntaxes(like create table options: CHARSET/COLLATE) are introduced without the dialect, but they are not valid syntax for many dialects. From my intuitive sense, we should add a dialect guard by default unless we know it's valid syntax for all dialects. Is that correct? please help to correct me if I'm wrong.

[git-hulk]

cc @alamb @iffyio @lovasoa

[alamb]

I think this sounds like a reasonable approach.

My opinion in general is that the key thing to do is to

1. Not add additional burden on new contributors
2. Not add add additional burden on reviewers (as this is currently the most limited resource)

So for example, just writing up a policy and expecting new contributors and reviewes to enforce it in my mind is not likely to be successful

However, if we refactored the code to use this model, then ensuring new code followed the exsiting code would be straightforward

What I think might be a better middle ground would be to gate any such syntax on some Dialect trait function (not a hard coded dialect_of!) which would largely follow existing patterns and allow users of sqlparser to update their custom dialect if it is important for their usecase to reject certain syntaxes

[lovasoa]

should add a dialect guard by default unless we know it's valid syntax for all dialects

@alamb , do you agree with that ? I don't think that's a good idea. There is as many SQL dialects as there are SQL databases, and we cannot expect to support all of them in this crate. I think the ability to create custom dialects outside of this crate is important. And in particular, the ability to create a dialect that inherits all the properties of another dialect, and just changes one small thing. Hardcoding supported dialects with dialect_of! (I guess this is what you meant by "dialect guards") breaks that.

And generally, I think accidentally accepting to create an AST for a syntax that is not valid in the current dialect is a much less serious problem than refusing to create an AST for a valid syntax. Users of this crate can always add custom rules to discard an AST that has been produced, but they cannot create an AST themselves if this crate returns an error.

For this reason, my opinion is that we should remove dialect_of! (new contributors couldn't use it if it wasn't there) and allow only the usage of trait methods in the Dialect trait. And not being too strict on having a method in Dialect for everything; only major syntax differences should be handled by the trait; small things can be accepted on every dialect.

[alamb]

should add a dialect guard by default unless we know it's valid syntax for all dialects

I don't think it is a good idea because:

1. It is not consistent with the current very permissive parsing behavior of SQLParser
2. User's can reject syntax they don't want to support as you say "Users of this crate can always add custom rules to discard an AST that has been produced"

For this reason, my opinion is that we should remove dialect_of! (new contributors couldn't use it if it wasn't there) and allow only the usage of trait methods in the Dialect trait. And not being too strict on having a method in Dialect for everything; only major syntax differences should be handled by the trait; small things can be accepted on every dialect.

In general this sounds like an ideal plan to me. However, I would caution anyone reading it that it would be a major under taking (there are a were over 100 uses of dialect_of when I just checked)

[git-hulk]

Thanks for @alamb and @lovasoa. I raise this discussion because I'm not quite sure which dialects might be affected without the dialect guard while changing the code. The points that @lovasoa mentioned also makes a lot of sense to me. It's clear to me now, thanks all again.