move primitive dialect configuration into a struct #1431

samuelcolvin · 2024-09-14T12:16:21Z

Step towards #1430.

I personally think this should make it much easier to add more flags to dialects. But I can see it might not be to everyone's taste.

So I want confirmation it's a good idea before prcoeeding.

This should:

make it less verbose to add more options to dialects
make it easier to define dialects that inherit from other dialects.

coveralls · 2024-09-14T12:21:07Z

Pull Request Test Coverage Report for Build 10862628784

Details

381 of 473 (80.55%) changed or added relevant lines in 32 files are covered.
855 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.08%) to 89.411%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/dialect/mod.rs	40	41	97.56%
src/lib.rs	0	1	0.0%
src/tokenizer.rs	63	64	98.44%
src/dialect/ansi.rs	4	6	66.67%
src/dialect/mssql.rs	4	7	57.14%
src/dialect/redshift.rs	6	9	66.67%
src/dialect/hive.rs	4	8	50.0%
src/dialect/mysql.rs	4	8	50.0%
src/dialect/sqlite.rs	4	8	50.0%
tests/sqlparser_custom_dialect.rs	8	12	66.67%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_redshift.rs	1	97.14%
src/dialect/mod.rs	1	78.03%
src/tokenizer.rs	1	94.24%
tests/sqlparser_duckdb.rs	2	99.57%
tests/sqlparser_bigquery.rs	8	90.32%
src/parser/mod.rs	10	93.31%
tests/sqlparser_snowflake.rs	10	98.06%
tests/sqlparser_databricks.rs	16	89.61%
tests/sqlparser_postgres.rs	213	88.64%
tests/sqlparser_common.rs	593	89.8%

Totals
Change from base Build 10817360923:	0.08%
Covered Lines:	29487
Relevant Lines:	32979

💛 - Coveralls

samuelcolvin

overall this PR is significantly negative LOC — showing that this approach is at least less verbose.

samuelcolvin · 2024-09-14T12:23:55Z

src/dialect/mod.rs

-            fn dialect(&self) -> std::any::TypeId {
+            fn settings(&self) -> DialectSettings {
+                let s = self.0.settings();
+                DialectSettings {


this is slightly ugly because WrappedDialect only implemented a few of the methods on the dialect.

But this actually demonstrates one of the advtanges of the approach in this PR:

If this test had been written with the new behaviour, this method would become

fn settings(&self) -> DialectSettings { let mut s = self.0.settings(); s.custom_thing = true; s }

And it would stay up-to-date even as new setting were added. Unlike the current behaviour.

I can update this test if you like?

samuelcolvin · 2024-09-14T12:24:42Z

src/dialect/mod.rs

            fn is_proper_identifier_inside_quotes(
                &self,
-                chars: std::iter::Peekable<std::str::Chars<'_>>,
+                chars: Peekable<Chars<'_>>,


it would be great if clippy or rustfmt could warn about these unnecessary paths, rustrover warns, hence the drive by change.

samuelcolvin · 2024-09-14T12:31:36Z

src/dialect/mod.rs

+    fn flags(&self) -> DialectFlags {
+        DialectFlags::default()
+    }


I don't like creating a new struct on every call (and there are a lot).

I'd rather store the struct on each dialect, then the signature here can before

fn flags(&self) -> &DialectFlags { &self.settings }

The problem then is that every struct needs a fn new() or impl Default, and the signature to create all dialects changes everywhere, but if we're happy with that breaking change, the whole thing would become much cleaner. WDYT?

samuelcolvin · 2024-09-14T13:03:06Z

Okay having said I would wait for feedback, I couldn't resist moving flags onto the structs.

While I'm aware there's a good chance this might get closed, I think this is the right way to go if we want something of this sort.

iffyio

@samuelcolvin Thanks for tackling this! The changes look very reasonable to me

cc @alamb also @jmhain I think you had a similar idea for this, in case you have any thoughts

iffyio · 2024-09-15T06:16:22Z

src/dialect/mod.rs

@@ -63,6 +63,152 @@ macro_rules! dialect_of {
    };
 }

+/// Constant settings for a dialect.
+///
+/// For configuration to be defined here, rather than on the rate:


on the rate

was this a typo?

iffyio · 2024-09-15T06:17:20Z

src/dialect/mod.rs

+/// * It must be a const setting, not value that's dependent on context - that has to be a function
+/// * have a default that matches `Default::default()`
+#[derive(Debug, Default, Clone)]
+pub struct DialectFlags {


Would it make sense to call it e.g. DialectSettings, DialectConstants or similar if we envision this potentially containing more than boolean flags?

Happy to change a name.

alamb

Thank you @samuelcolvin and @iffyio

In my opinion:

Adding impl Default for the dialects is nice as it is sort of a usability papercut (it would also nice to have a new() function, to follow convention)
In terms of DialectFlags I personally think the improvement is not worth the API churn, though I don't really know how many people have their own Dialect trait implementations 🤔

samuelcolvin · 2024-09-19T19:01:20Z

I personally think the improvement is not worth the API churn

Okay, I'll leave this open for a few days in case anyone else is particularly keen for it, otherwise I'll close this and work on #1430 by adding lots of boolean functions.

alamb · 2024-09-19T19:35:58Z

Okay, I'll leave this open for a few days in case anyone else is particularly keen for it, otherwise I'll close this and work on #1430 by adding lots of boolean functions.

Maybe we should contemplate creating a parser that parameterized on the dialect (Parser<Dialect> -- I know we have discussed this before and it likely isn't a huge deal)

jmhain · 2024-09-19T21:32:52Z

Maybe we should contemplate creating a parser that parameterized on the dialect (Parser -- I know we have discussed this before and it likely isn't a huge deal)

This would cause a lot of code bloat for use cases that require parsing many different dialects. What's the benefit of doing it?

samuelcolvin · 2024-09-19T22:43:38Z

What do you mean by "a lot of code bloat"?

Surely you can create one parser for each dialect? The memory overhead would be tiny.

Avoiding dynamic dispatch would lead to better performance, smaller binaries and (most importantly) more canonical code in the library.

alamb · 2024-09-20T16:43:41Z

What do you mean by "a lot of code bloat"?

Surely you can create one parser for each dialect? The memory overhead would be tiny.

Avoiding dynamic dispatch would lead to better performance, smaller binaries and (most importantly) more canonical code in the library.

I think the idea is if we made a parser you could end up with multiple copies of the Parser code for each one you instantiated -- like Parser<GenericDialect> and Parser<BigQueryDialect>

I think one workaround would be to make your own DynamicDialect that dispatched dynamically and have Parser<DynamicDialect>

However I think I may have lead us offtopic here. The dynamic dispatch has always kind of annoyed me but I have never had any actual justification to change it. Sorry for starting this discusision

alamb · 2024-10-20T18:13:59Z

Converting to a draft as I don't think this PR is waiting on review anymore

github-actions · 2024-12-20T02:01:41Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

samuelcolvin · 2024-12-20T07:00:01Z

Closing as I don't think we're going this way anymore.

move primative dialect configuration into a struct

b4d7181

samuelcolvin commented Sep 14, 2024

View reviewed changes

linting settings -> flags

929af3e

samuelcolvin commented Sep 14, 2024

View reviewed changes

switch flags to be stored on the dialect

a445a87

fmt

9f89fb6

samuelcolvin marked this pull request as ready for review September 14, 2024 13:26

iffyio reviewed Sep 15, 2024

View reviewed changes

alamb reviewed Sep 19, 2024

View reviewed changes

alamb marked this pull request as draft October 20, 2024 18:13

github-actions bot added the Stale label Dec 20, 2024

samuelcolvin closed this Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move primitive dialect configuration into a struct #1431

move primitive dialect configuration into a struct #1431

samuelcolvin commented Sep 14, 2024

coveralls commented Sep 14, 2024 •

edited

Loading

samuelcolvin left a comment

samuelcolvin Sep 14, 2024

samuelcolvin Sep 14, 2024

samuelcolvin Sep 14, 2024

samuelcolvin Sep 14, 2024

samuelcolvin commented Sep 14, 2024

iffyio left a comment

iffyio Sep 15, 2024

iffyio Sep 15, 2024

samuelcolvin Sep 15, 2024

alamb left a comment

samuelcolvin commented Sep 19, 2024

alamb commented Sep 19, 2024

jmhain commented Sep 19, 2024

samuelcolvin commented Sep 19, 2024

alamb commented Sep 20, 2024

alamb commented Oct 20, 2024

github-actions bot commented Dec 20, 2024

samuelcolvin commented Dec 20, 2024

move primitive dialect configuration into a struct #1431

move primitive dialect configuration into a struct #1431

Conversation

samuelcolvin commented Sep 14, 2024

coveralls commented Sep 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10862628784

Details

💛 - Coveralls

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin Sep 14, 2024

Choose a reason for hiding this comment

samuelcolvin Sep 14, 2024

Choose a reason for hiding this comment

samuelcolvin Sep 14, 2024

Choose a reason for hiding this comment

samuelcolvin Sep 14, 2024

Choose a reason for hiding this comment

samuelcolvin commented Sep 14, 2024

iffyio left a comment

Choose a reason for hiding this comment

iffyio Sep 15, 2024

Choose a reason for hiding this comment

iffyio Sep 15, 2024

Choose a reason for hiding this comment

samuelcolvin Sep 15, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

samuelcolvin commented Sep 19, 2024

alamb commented Sep 19, 2024

jmhain commented Sep 19, 2024

samuelcolvin commented Sep 19, 2024

alamb commented Sep 20, 2024

alamb commented Oct 20, 2024

github-actions bot commented Dec 20, 2024

samuelcolvin commented Dec 20, 2024

coveralls commented Sep 14, 2024 •

edited

Loading