support general typed string literals by maxcountryman · Pull Request #187 · apache/datafusion-sqlparser-rs

maxcountryman · 2020-06-06T16:51:12Z

This is a direct port of MaterializeInc/materialize#3146. It provides
support for e.g. Postgres syntax where any string literal may be
preceded by a type name.

Fixes #168.

nickolay

Thanks for doing this! The comments below are mainly nits, as usual, but I do think it's important to switch back to using a TypedString (a la MaterializeInc/materialize@d6b24fb) instead of converting DATE 'yyyy-mm-dd to a Cast, and to fix the NOT 'a' like 'b' bug (possibly by disallowing the custom data types as part of a "typed string" - MaterializeInc's version of the parser seems to have disabled this feature at all).

nickolay · 2020-06-07T07:46:56Z

I wanted to note that even though that Materialize as a whole is licensed under a non-opensource license, they kept the sql-parser bits we borrow these changes from (value.rs and parser.rs) available under the Apache license, so there shouldn't be a problem with this.

maxcountryman · 2020-06-08T13:44:12Z

Thank you for the feedback! I'll try to have another look at this later on this week.

maxcountryman · 2020-06-10T18:36:28Z

 }

+// Returns a successful result if the optional expression is some
+macro_rules! return_ok_if_some {


Let me know what you think about this name: it seemed a little closer to what's happening.

I thought maybe_parse could return a Result (return_if_ok!(self.maybe_parse(|parser| {), but this is probably fine too.

Oh sure, happy to do it that way if you prefer!

nickolay · 2020-06-10T19:14:07Z

 }

+// Returns a successful result if the optional expression is some
+macro_rules! return_ok_if_some {


I thought maybe_parse could return a Result (return_if_ok!(self.maybe_parse(|parser| {), but this is probably fine too.

nickolay · 2020-06-10T20:16:15Z

+            // Single-quoted strings are parsed as custom data types, however this not desirable
+            // when we are handling input like `"NOT 'a' NOT LIKE 'b'"` because this will produce a


This doesn't seem right. Any name (Word in our parlance), not a 'string', will be accepted as a custom data type, even when it's a keyword that should be handled by the code below.

nickolay · 2020-06-10T20:18:08Z

+            DataType::Interval => parser.parse_literal_interval(),
+            // Single-quoted strings are parsed as custom data types, however this not desirable
+            // when we are handling input like `"NOT 'a' NOT LIKE 'b'"` because this will produce a
+            // TypedString instead of a SingleQuotedString. Further, this leads to issues where the


a TypedString[NOT 'a'] instead of Unary NOT?

nickolay · 2020-06-10T20:19:43Z

+            // Single-quoted strings are parsed as custom data types, however this not desirable
+            // when we are handling input like `"NOT 'a' NOT LIKE 'b'"` because this will produce a
+            // TypedString instead of a SingleQuotedString. Further, this leads to issues where the
+            // same input will yield a BinaryOperator instead of the correct UnaryOperator. Here we


I don't understand this. If it's important, an example to demonstrate it would be welcome.

I can remove this, it's just explaining why our test would fail if we didn't explicitly handle DataType::Custom by returning an error (e.g. it will parse as a BinaryOperator instead of an UnaryOperator).

Would it be better to change this entire comment to say something to the effect of we don't accept the type 'string' syntax for the custom data types? (Borrowing from your comment below.)

It will parse as a BinaryOperator instead of an UnaryOperator

I don't understand what will parse as BinaryOperator.

Would it be better to change this entire comment to say something to the effect of we don't accept the type 'string' syntax for the custom data types? (Borrowing from your comment below.)

I think both the description of what returning an Err() here does and the non-obvious example of NOT 'x' LIKE 'y' should stay in the comment. I'm just trying to make sure I understand the examples and that they are clearly described.

I don't understand what will parse as BinaryOperator.

Here I mean the direct example given in the comment of "NOT 'a' NOT LIKE 'b'".

I think both the description of what returning an Err() here does and the non-obvious example of NOT 'x' LIKE 'y' should stay in the comment. I'm just trying to make sure I understand the examples and that they are clearly described.

Sure, makes sense. I may be approaching the limits of my (admittedly shallow) understanding of the library here so I may need to defer to you for how you'd like to document that.

Here I mean the direct example given in the comment of "NOT 'a' NOT LIKE 'b'".

Ah, sorry for being dense.

nickolay · 2020-06-10T20:24:08Z

+            // Single-quoted strings are parsed as custom data types, however this not desirable
+            // when we are handling input like `"NOT 'a' NOT LIKE 'b'"` because this will produce a
+            // TypedString instead of a SingleQuotedString. Further, this leads to issues where the
+            // same input will yield a BinaryOperator instead of the correct UnaryOperator. Here we
+            // handle that specific case by returning an error.


"...we don't accept the type 'string' syntax for the custom data types at all."

nickolay · 2020-06-10T20:30:49Z

+            // Check if the recently consumed '(' started a derived table, in which case we've
+            // parsed the subquery, followed by the closing ')', and the alias of the derived
+            // table. In the example above this is case (3).
+            //
+            // A parsing error from `parse_derived_table_factor` indicates that the '(' we've
+            // recently consumed does not start a derived table (cases 1, 2, or 4). Ignore the
+            // error and back up to where we after the opening '('.


The comments' text does not match their new location in the code. I can update the wording if you like.

Sure, that would be helpful if you don't mind. 🙂

This is a direct port of MaterializeInc/materialize#3146. It provides support for e.g. Postgres syntax where any string literal may be preceded by a type name. Fixes apache#168.

coveralls · 2020-06-11T13:31:20Z

Pull Request Test Coverage Report for Build 132531077

41 of 41 (100.0%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.3%) to 91.708%

Files with Coverage Reduction	New Missed Lines	%
src/ast/value.rs	1	87.04%
src/parser.rs	1	89.27%

Totals
Change from base Build 132518608:	0.3%
Covered Lines:	4070
Relevant Lines:	4438

💛 - Coveralls

maxcountryman · 2020-06-11T13:31:21Z

I rebased this off of master (and squashed to avoid having to resolve the merge conflicts in each commit) so this should now incorporate the mainline changes.

nickolay · 2020-06-11T16:41:56Z

I rebased this off of master (and squashed to avoid having to resolve the merge conflicts in each commit) so this should now incorporate the mainline changes.

Excellent.

I pushed an update to the comments, let me know if they make sense, and I think this is ready to merge?

maxcountryman · 2020-06-11T19:32:32Z

+                // PosgreSQL allows almost any identifier to be used as custom data type name,
+                // and we support that in `parse_data_type()`. But unlike Postgres we don't
+                // have a list of globally reserved keywords (since they vary across dialects),
+                // so given `NOT 'a' LIKE 'b'`, we'd accept `NOT` as a possible custom data type
+                // name, resulting in `NOT 'a'` being recognized as a `TypedString` instead of
+                // an unary negation `NOT ('a' LIKE 'b')`. To solve this, we don't accept the
+                // `type 'string'` syntax for the custom data types at all.


This reads so much better. Thank you!

maxcountryman · 2020-06-11T19:34:28Z

I pushed an update to the comments, let me know if they make sense, and I think this is ready to merge?

Amazing. Thank you! It looks good to me. Really appreciate all the detailed feedback and of course patience. 🙂

nickolay · 2020-06-11T21:04:51Z

Thank you as well!

maxcountryman force-pushed the feature/port-materialize-#3146 branch from 8487d2e to 731f400 Compare June 6, 2020 16:55

nickolay suggested changes Jun 7, 2020

View reviewed changes

nickolay mentioned this pull request Jun 7, 2020

rework github actions for code coverage #186

Merged

nickolay reviewed Jun 10, 2020

View reviewed changes

Comment thread src/parser.rs Outdated

maxcountryman commented Jun 10, 2020

View reviewed changes

Comment thread src/parser.rs Outdated

nickolay suggested changes Jun 10, 2020

View reviewed changes

maxcountryman force-pushed the feature/port-materialize-#3146 branch from 4b4e73d to 2fa9479 Compare June 11, 2020 13:26

support general typed string literal

51cb5c7

This is a direct port of MaterializeInc/materialize#3146. It provides support for e.g. Postgres syntax where any string literal may be preceded by a type name. Fixes apache#168.

maxcountryman force-pushed the feature/port-materialize-#3146 branch from 2fa9479 to 51cb5c7 Compare June 11, 2020 13:29

Doc updates

cef2924

Merge branch 'master' into feature/port-materialize-#3146

0fdeeca

maxcountryman commented Jun 11, 2020

View reviewed changes

nickolay merged commit 6cdd4a1 into apache:master Jun 11, 2020

maxcountryman deleted the feature/port-materialize-#3146 branch June 11, 2020 22:02

		// Single-quoted strings are parsed as custom data types, however this not desirable
		// when we are handling input like `"NOT 'a' NOT LIKE 'b'"` because this will produce a

Conversation

maxcountryman commented Jun 6, 2020

Uh oh!

nickolay left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickolay commented Jun 7, 2020

Uh oh!

maxcountryman commented Jun 8, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickolay Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 132531077

💛 - Coveralls

Uh oh!

maxcountryman commented Jun 11, 2020

Uh oh!

nickolay commented Jun 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxcountryman commented Jun 11, 2020

Uh oh!

nickolay commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickolay left a comment •

edited

Loading

nickolay Jun 11, 2020 •

edited

Loading

coveralls commented Jun 11, 2020 •

edited

Loading