feat: Support escaped string literals (PostgreSQL) #502

ovr · 2022-05-18T18:47:47Z

Hello!

It's a draft which implements special PostgreSQL escaped string syntax.

https://www.postgresql.org/docs/8.3/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS

PostgreSQL also accepts "escape" string constants, which are an extension to the SQL standard. An escape string constant is specified by writing the letter E (upper or lower case) just before the opening single quote, e.g. E'foo'. (When continuing an escape string constant across lines, write E only before the first opening quote.) Within an escape string, a backslash character () begins a C-like backslash escape sequence, in which the combination of backslash and following character(s) represents a special byte value. \b is a backspace, \f is a form feed, \n is a newline, \r is a carriage return, \t is a tab.

Thanks

Signed-off-by: Dmitry Patsura <talk@dmtry.me>

ovr · 2022-05-18T18:48:11Z

@alamb can you take a look to verify that it's a correct approach. Thanks

coveralls · 2022-05-18T18:51:52Z

Pull Request Test Coverage Report for Build 2378670235

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

65 of 92 (70.65%) changed or added relevant lines in 4 files are covered.
553 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.9%) to 89.635%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/parser.rs	3	5	60.0%
src/ast/value.rs	11	19	57.89%
src/tokenizer.rs	29	46	63.04%

Files with Coverage Reduction	New Missed Lines	%
tests/sqlparser_redshift.rs	1	98.11%
tests/sqlparser_snowflake.rs	2	96.43%
tests/sqlparser_postgres.rs	16	97.83%
tests/sqlparser_common.rs	69	97.01%
src/ast/mod.rs	156	78.21%
src/parser.rs	309	82.96%

Totals
Change from base Build 2328175823:	-0.9%
Covered Lines:	8388
Relevant Lines:	9358

💛 - Coveralls

alamb · 2022-05-19T11:03:59Z

Thanks -- I am a little behind in sqlparser-rs review. I will try and find time this weekend to review

…

On Wed, May 18, 2022 at 2:52 PM Coveralls ***@***.***> wrote: Pull Request Test Coverage Report for Build 2347539602 <https://coveralls.io/builds/49246117> - *63* of *80* *(78.75%)* changed or added relevant lines in *4* files are covered. - *6* unchanged lines in *2* files lost coverage. - Overall coverage decreased (*-0.1%*) to *90.384%* ------------------------------ Changes Missing Coverage Covered Lines Changed/Added Lines % src/parser.rs <https://coveralls.io/builds/49246117/source?filename=src%2Fparser.rs#L908> 4 6 66.67% src/ast/value.rs <https://coveralls.io/builds/49246117/source?filename=src%2Fast%2Fvalue.rs#L207> 12 18 66.67% src/tokenizer.rs <https://coveralls.io/builds/49246117/source?filename=src%2Ftokenizer.rs#L402> 31 40 77.5% Files with Coverage Reduction New Missed Lines % src/parser.rs <https://coveralls.io/builds/49246117/source?filename=src%2Fparser.rs#L909> 1 82.94% src/tokenizer.rs <https://coveralls.io/builds/49246117/source?filename=src%2Ftokenizer.rs#L775> 5 89.5% Totals [image: Coverage Status] <https://coveralls.io/builds/49246117> Change from base Build 2328175823 <https://coveralls.io/builds/49129477>: -0.1% Covered Lines: 8365 Relevant Lines: 9255 ------------------------------ 💛 - Coveralls <https://coveralls.io> — Reply to this email directly, view it on GitHub <#502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXZMNOF6E5YP7YTXV5CJ3VKU35NANCNFSM5WJMUQGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alamb

Thank you @ovr -- I like this PR; very nice.

The only comment I think needs to be addressed prior to merge is the comment on EscapeEscapedStringLiteral

src/ast/value.rs

alamb · 2022-05-22T19:49:58Z

src/ast/value.rs

+
+impl<'a> fmt::Display for EscapeEscapedStringLiteral<'a> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let mut is_escaped = true;


I don't see is_escaped ever set to false -- I would expect it would start with is_escaped = true and then is_escaped wold be set to false after each character was written

How would this work with: First \n second \\ third \n fourth \

don't see is_escaped ever set to false -- I would expect it would start with is_escaped = true and then is_escaped wold be set to false after each character was written

I will remove it, It's useles.

It's not a correct value, because the last \ should be escaped.

alamb · 2022-05-22T19:50:16Z

src/parser.rs

@@ -496,6 +496,10 @@ impl<'a> Parser<'a> {
                    expr: Box::new(self.parse_subexpr(Self::PLUS_MINUS_PREC)?),
                })
            }
+            Token::EscapedStringLiteral(_) if dialect_of!(self is PostgreSqlDialect) => {


💯 for conditionalizing on postgres dialect

src/tokenizer.rs

alamb · 2022-05-22T19:56:16Z

src/tokenizer.rs

+        let mut s = String::new();
+        chars.next(); // consume the opening quote
+
+        // slash escaping


FYIW this example from stack overflow looks like it might be a nice way to avoid macro overhead (and thus code bloat): https://stackoverflow.com/questions/58551211/how-do-i-interpret-escaped-characters-in-a-string

It's not the same because this function tries to find & escapes the string from the query. It tries to find a single quote that can be escaped or not escaped (end of the string).

in our case string are wrapped in single quotes, i.e e'str'

alamb · 2022-05-22T19:59:37Z

tests/sqlparser_postgres.rs

+
+#[test]
+fn parse_escaped_literal_string() {
+    let sql = r#"SELECT E's1 \n s1', E's2 \\n s2', E's3 \\\n s3', E's4 \\\\n s4', E'\''"#;


I also recommend some negative tests like ' Foo\'

It's not a correct value because the last quote was escaped, there is no single quote which should close the string expr. f46b07e

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb

Looks good -- thank you @ovr !

Signed-off-by: Dmitry Patsura <talk@dmtry.me> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Can drop this after rebase on commit 2c0886d "feat: Support escaped string literals (PostgreSQL) (apache#502)", first released in 0.18.0

feat: Support escaped string literals (PostgreSQL)

6ad3b26

Signed-off-by: Dmitry Patsura <talk@dmtry.me>

lint

c71af02

escape ', \r, \t

c067717

ovr marked this pull request as ready for review May 20, 2022 14:33

alamb approved these changes May 22, 2022

View reviewed changes

ovr and others added 6 commits May 23, 2022 13:47

Update src/ast/value.rs

85dfffe

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update src/tokenizer.rs

d4498e2

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

test: two slashes

46d94f6

remove dead code

d93ec4d

test: parsing error

f46b07e

support generic dialect too (for DF)

5d454e4

alamb approved these changes May 25, 2022

View reviewed changes

alamb merged commit 2c0886d into apache:main May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Support escaped string literals (PostgreSQL) #502

feat: Support escaped string literals (PostgreSQL) #502

Uh oh!

ovr commented May 18, 2022

Uh oh!

ovr commented May 18, 2022

Uh oh!

coveralls commented May 18, 2022 •

edited

Loading

Uh oh!

alamb commented May 19, 2022 via email

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

alamb May 22, 2022

Uh oh!

alamb May 22, 2022

Uh oh!

ovr May 24, 2022

Uh oh!

alamb May 22, 2022

Uh oh!

Uh oh!

alamb May 22, 2022

Uh oh!

ovr May 24, 2022 •

edited

Loading

Uh oh!

alamb May 22, 2022

Uh oh!

ovr May 24, 2022

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

feat: Support escaped string literals (PostgreSQL) #502

feat: Support escaped string literals (PostgreSQL) #502

Uh oh!

Conversation

ovr commented May 18, 2022

Uh oh!

ovr commented May 18, 2022

Uh oh!

coveralls commented May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2378670235

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

alamb commented May 19, 2022 via email

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb May 22, 2022

Choose a reason for hiding this comment

Uh oh!

alamb May 22, 2022

Choose a reason for hiding this comment

Uh oh!

ovr May 24, 2022

Choose a reason for hiding this comment

Uh oh!

alamb May 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb May 22, 2022

Choose a reason for hiding this comment

Uh oh!

ovr May 24, 2022 • 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!

alamb May 22, 2022

Choose a reason for hiding this comment

Uh oh!

ovr May 24, 2022

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented May 18, 2022 •

edited

Loading

ovr May 24, 2022 •

edited

Loading