-
Notifications
You must be signed in to change notification settings - Fork 2k
Do not unescape backslashes in datafusion-cli #14844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
e3c56d4
87790e3
d304cfc
a4d7a10
8c1785b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,28 +170,44 @@ impl Helper for CliHelper {} | |
| /// | ||
| /// 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> { | ||
| let mut chars = input.chars(); | ||
|
|
||
| let mut chars = input.chars().peekable(); | ||
| let mut result = String::with_capacity(input.len()); | ||
| while let Some(char) = chars.next() { | ||
| if char == '\\' { | ||
| if let Some(next_char) = chars.next() { | ||
| // https://static.rust-lang.org/doc/master/reference.html#literals | ||
| result.push(match next_char { | ||
| '0' => '\0', | ||
| 'n' => '\n', | ||
| 'r' => '\r', | ||
| 't' => '\t', | ||
| '\\' => '\\', | ||
|
|
||
| while let Some(ch) = chars.next() { | ||
| if ch == '\\' { | ||
| if let Some(&next) = chars.peek() { | ||
| match next { | ||
| '0' => { | ||
| chars.next(); | ||
| result.push('\0'); | ||
| } | ||
| 'n' => { | ||
| chars.next(); | ||
| result.push('\n'); | ||
| } | ||
| 'r' => { | ||
| chars.next(); | ||
| result.push('\r'); | ||
| } | ||
| 't' => { | ||
| chars.next(); | ||
| result.push('\t'); | ||
| } | ||
| '\\' | '\'' => result.push('\\'), | ||
| _ => { | ||
| return Err(sql_datafusion_err!(ParserError::TokenizerError( | ||
| format!("unsupported escape char: '\\{}'", next_char) | ||
| return Err(DataFusionError::Execution(format!( | ||
| "Invalid escape sequence: \\{}", | ||
| next | ||
| ))) | ||
| } | ||
| }); | ||
| } | ||
| } else { | ||
| return Err(sql_datafusion_err!(ParserError::TokenizerError( | ||
| "incomplete escape sequence: trailing backslash".to_string() | ||
| ))); | ||
| } | ||
| } else { | ||
| result.push(char); | ||
| result.push(ch); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -310,14 +326,13 @@ mod tests { | |
| )?; | ||
| assert!(matches!(result, ValidationResult::Valid(None))); | ||
|
|
||
| // should be invalid | ||
| let result = readline_direct( | ||
| Cursor::new( | ||
| r"create external table test stored as csv location 'data.csv' options ('format.delimiter' '\u{07}');" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worth keeping this test case?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| .as_bytes()), | ||
| &validator, | ||
| )?; | ||
| assert!(matches!(result, ValidationResult::Invalid(Some(_)))); | ||
| Cursor::new( | ||
| r"select '\', '\\', '\\\\\', 'dsdsds\\\\', '\t', '\0', '\n';".as_bytes(), | ||
| ), | ||
| &validator, | ||
| )?; | ||
| assert!(matches!(result, ValidationResult::Valid(None))); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,10 @@ fn init() { | |
| ["--command", "select 1; select 2;", "--format", "json", "-q"], | ||
| "[{\"Int64(1)\":1}]\n[{\"Int64(2)\":2}]\n" | ||
| )] | ||
| #[case::exec_backslash( | ||
| ["--file", "tests/data/backslash.txt", "--format", "json", "-q"], | ||
| "[{\"Utf8(\\\"\\\\\\\")\":\"\\\\\",\"Utf8(\\\"\\\\\\\\\\\")\":\"\\\\\\\\\",\"Utf8(\\\"\\\\\\\\\\\\\\\\\\\\\\\")\":\"\\\\\\\\\\\\\\\\\\\\\",\"Utf8(\\\"dsdsds\\\\\\\\\\\\\\\\\\\")\":\"dsdsds\\\\\\\\\\\\\\\\\",\"Utf8(\\\"\\t\\\")\":\"\\t\",\"Utf8(\\\"\\u0000\\\")\":\"\\u0000\",\"Utf8(\\\"\\n\\\")\":\"\\n\"}]\n" | ||
| )] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's so many backslashes on a single line. |
||
| #[case::exec_from_files( | ||
| ["--file", "tests/data/sql.txt", "--format", "json", "-q"], | ||
| "[{\"Int64(1)\":1}]\n" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| select '\', '\\', '\\\\\', 'dsdsds\\\\', '\t', '\0', '\n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this check since it would throw error later
