Simple custom lexical precedence in PostgreSQL dialect

[samuelcolvin]

As suggest mentioned by @lovasoa in #1360 (comment) the previous approach had unnecessary duplication, this should reduce the duplication of logic while still allowing complete freedom for how dialects decide precedence.

This doesn't change the logic for PostgreSqlDialect except that Token::DuckIntDiv will now return MulDivModOp precedence, not UNKOWN.

[lovasoa]

🎉

[coveralls]

Pull Request Test Coverage Report for Build 10387878575

Details

• 88 of 93 (94.62%) changed or added relevant lines in 4 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.07%) to 89.124%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dialect/postgresql.rs	22	23	95.65%
src/dialect/mod.rs	50	54	92.59%

Files with Coverage Reduction	New Missed Lines	%
src/dialect/mod.rs	2	68.6%

Totals
Change from base Build 10370172981:	0.07%
Covered Lines:	27641
Relevant Lines:	31014

---

💛 - Coveralls

[alamb]

Thank you @samuelcolvin -- this looks like an improvement to me. Thank you for the follow up

Since we haven't released the changes in #1360 to crates.io yet, this isn't an API change

I'll wait a day before merging this in case @lovasoa would like to review or comment on this PR as well

Update: I see @lovasoa has already approved it. So let's do this 🚀

[alamb]

I think this is a good idea.

My only potential concern with this approach is that it could be potentially slower, as it now has many more function calls, however I don't actually think we have a sqlparser benchmark so at this point so trying to make such a change would likely be a pre-mature optimization.

If anyone needs additional performance, we can do that as follow on PRs

[alamb]

❤️

[samuelcolvin]

thanks a lot.

[samuelcolvin]

@alamb if we care about performance, we should stop using dynamic dispatch and make the parser generic around the dialect, with that we could make lots of these methods const (or drop the Precedence enum and just have const values on the trait) and probably improve performance significantly in general.

That would of course be a big change to the public API, but worthwhile IMHO.

[alamb]

@alamb if we care about performance, we should stop using dynamic dispatch and make the parser generic around the dialect, with that we could make lots of these methods const (or drop the Precedence enum and just have const values on the trait) and probably improve performance significantly in general.

That would of course be a big change to the public API, but worthwhile IMHO.

I agree with both points

[lovasoa]

In my use case (SQLPage), SQL parsing overhead is negligible. Have you ever been bottlenecked by sql parsing?

[alamb]

In my use case (SQLPage), SQL parsing overhead is negligible. Have you ever been bottlenecked by sql parsing?

No. There is more discussion here as well: #1381 (comment)