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 2379093604

69 of 97 (71.13%) changed or added relevant lines in 4 files are covered.
601 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.8%) to 89.639%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tests/sqlparser_postgres.rs	24	25	96.0%
src/parser.rs	4	7	57.14%
src/ast/value.rs	11	19	57.89%
src/tokenizer.rs	30	46	65.22%

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.71%
tests/sqlparser_common.rs	69	97.01%
src/ast/mod.rs	156	78.21%
src/parser.rs	357	82.97%

Totals
Change from base Build 2328175823:	-0.8%
Covered Lines:	8392
Relevant Lines:	9362

💛 - 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 !

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 sqlparser-rs:main May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support escaped string literals (PostgreSQL) #502

feat: Support escaped string literals (PostgreSQL) #502

ovr commented May 18, 2022

ovr commented May 18, 2022

coveralls commented May 18, 2022 •

edited

alamb commented May 19, 2022 via email

alamb left a comment

alamb May 22, 2022

alamb May 22, 2022

ovr May 24, 2022

alamb May 22, 2022

alamb May 22, 2022

ovr May 24, 2022 •

edited

alamb May 22, 2022

ovr May 24, 2022

alamb left a comment

feat: Support escaped string literals (PostgreSQL) #502

feat: Support escaped string literals (PostgreSQL) #502

Conversation

ovr commented May 18, 2022

ovr commented May 18, 2022

coveralls commented May 18, 2022 • edited

Pull Request Test Coverage Report for Build 2379093604

💛 - Coveralls

alamb commented May 19, 2022 via email

alamb left a comment

Choose a reason for hiding this comment

alamb May 22, 2022

Choose a reason for hiding this comment

alamb May 22, 2022

Choose a reason for hiding this comment

ovr May 24, 2022

Choose a reason for hiding this comment

alamb May 22, 2022

Choose a reason for hiding this comment

alamb May 22, 2022

Choose a reason for hiding this comment

ovr May 24, 2022 • edited

Choose a reason for hiding this comment

alamb May 22, 2022

Choose a reason for hiding this comment

ovr May 24, 2022

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

coveralls commented May 18, 2022 •

edited

ovr May 24, 2022 •

edited