correctly treat backslash in datafusion-cli #14844
Conversation
Lordworms
force-pushed
the
issue_13286
branch
from
240b43f to
08cf1c8
Compare
|
|
Lordworms
force-pushed
the
issue_13286
branch
from
08cf1c8 to
e3c56d4
Compare
There was a problem hiding this comment.
Hi @Lordworms -- thank you for this PR
I played around with it a bit locally and something still doesn't seem right
Specifically, postgres will treat \1 like a literal string while this PR still tries to escape it...
postgres=# select '\1';
?column?
----------
\1
(1 row)But when I try in this PR:
select 'DataFusion CLI v45.0.0
> select '\';
+-----------+
| Utf8("\") |
+-----------+
| \ |
+-----------+
1 row(s) fetched.
Elapsed 0.013 seconds.
> select '\1'; 🤔 Invalid statement: Execution error: Invalid escape sequence: \1
I think this is correct, because postgresql treats line breaks, such as \t \0, as plain text, but in the original design of DataFusion, \t, \0 should be escaped, so for \1, since it cannot be escaped, an error will be thrown. I think the original purpose of this issue should be to fix single or multiple backslashes. I am not sure whether we should treat all input as plain text like postgresql. 🤔 |
I think you are right that this is the core question: "should If we want to behave like sqllogictest / postgres I think the answer is no. The escaping should happen with the sql supported syntax such as Here is a proposed change to this PR that I think fixes the bug by removing unescaping in datafusion-cli: I am not sure what I think about this to be honest. |
| @@ -326,15 +326,6 @@ mod tests { | |||
| )?; | |||
| assert!(matches!(result, ValidationResult::Valid(None))); | |||
|
|
|||
| // should be invalid | |||
There was a problem hiding this comment.
removed this check since it would throw error later
Remove all cli unescaping
no, the CLI should not do thos
yes, this should happen during/after SQL AST is built in particular, there could be different kind of string literals: such that treat |
| #[case::exec_backslash( | ||
| ["--file", "tests/data/backslash.txt", "--format", "json", "-q"], | ||
| "[{\"Utf8(\\\"\\\\\\\")\":\"\\\\\",\"Utf8(\\\"\\\\\\\\\\\")\":\"\\\\\\\\\",\"Utf8(\\\"\\\\\\\\\\\\\\\\\\\\\\\")\":\"\\\\\\\\\\\\\\\\\\\\\",\"Utf8(\\\"dsdsds\\\\\\\\\\\\\\\\\\\")\":\"dsdsds\\\\\\\\\\\\\\\\\",\"Utf8(\\\"\\\\t\\\")\":\"\\\\t\",\"Utf8(\\\"\\\\0\\\")\":\"\\\\0\",\"Utf8(\\\"\\\\n\\\")\":\"\\\\n\"}]\n" | ||
| )] |
There was a problem hiding this comment.
That's so many backslashes on a single line.
What does it test? How do we know it's really correct?
(when i test's semantics is debatable, it's prone to change later; it's better to have a test that "is obviously correct")
| /// The data read from stdio will be escaped, so we need to unescape the input before executing the input | ||
| pub fn unescape_input(input: &str) -> datafusion::error::Result<String> { |
There was a problem hiding this comment.
So this function was the source of the bug. Per the comment above, someone had a reason to write it though. Do you maybe know what changed? Like, did we change how we obtain input lines?
There was a problem hiding this comment.
One thing that might have changed is that sqlparser got much more sophistcated in handling escaping itself / added a bunch of handling for the escaping in the parser (like maybe \u{} style syntax 🤔 )
| let result = readline_direct( | ||
| Cursor::new( | ||
| r"create external table test stored as csv location 'data.csv' options ('format.delimiter' '\u{07}');" |
There was a problem hiding this comment.
no, the check is to validate delimiter in create csv files using the unescape function. It will be verified when the external table is created later.
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?