-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
SQL: Add option to provide the delimiter for the CSV format #59907
Conversation
This adds the option to provide the desired character as the separator for the CSV format (the default remains comma). A set of characters are excluded though - like CR, LF, `"` - to avoid slipping onto the CSV-dialects slope. The tab is also forbidden, the user needs to choose the "tsv" format explicitely. The TSV format also gets a needed supplemental escaping, that of the '\' character.
@elasticmachine run elasticsearch-ci/2 |
Pinging @elastic/es-ql (:Query Languages/SQL) |
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.
Looks good, left a few comments, but additionally should something be adjusted in docs?
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java
Outdated
Show resolved
Hide resolved
Fix the way the delimiter is being passed around, removing the CSV member - since its instantiation is a singleton - and pushing it through function arguments. The commit fixes a few issues with the "header" URL attribute: - these was no longer usable, since it wasn't "consumed" by RestSqlQueryAction before generating the answer (triggering an error responce on use); - its value was only checked against "absent", so an incorrect value (typo like "abaent") could have silently generated an unexpected reply. - missing Documentation for the "delimiter" attribute has also been added.
Of course, missed that, thanks for pointing it out. |
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.
LGTM. Left a few more minor comments.
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java
Show resolved
Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java
Outdated
Show resolved
Hide resolved
The commit unescapes the deliter param. This is required for the doc-sourced tests. Also, implement a switch for param value validation and fix docs.
@elasticmachine update branch |
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.
LGTM, Thx!
Left one more minor comment in docs.
Co-authored-by: Marios Trivyzas <matriv@gmail.com>
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.
LGTM in general, but I have doubts about adding the header
request parameter.
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java
Outdated
Show resolved
Hide resolved
- rip out anything which isn't Delimiter-related. - undo 'hasHeader' method renaming.
@elasticmachine update branch |
Update the doc to make it clear that the textual CSV, TSV and TXT formats pass the cursor back to the user through the Cursor HTTP header.
@elasticmachine update branch |
1 similar comment
@elasticmachine update branch |
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.
LGTM
…59907) * Add option to provide the delimiter to the CSV fmt This adds the option to provide the desired character as the separator for the CSV format (the default remains comma). A set of characters are excluded though - like CR, LF, `"` - to avoid slipping onto the CSV-dialects slope. The tab is also forbidden, the user needs to choose the "tsv" format explicitely. Update the doc to make it clear that the textual CSV, TSV and TXT formats pass the cursor back to the user through the Cursor HTTP header. (cherry picked from commit 3a8b00c)
…60420) * SQL: Add option to provide the delimiter for the CSV format (#59907) * Add option to provide the delimiter to the CSV fmt This adds the option to provide the desired character as the separator for the CSV format (the default remains comma). A set of characters are excluded though - like CR, LF, `"` - to avoid slipping onto the CSV-dialects slope. The tab is also forbidden, the user needs to choose the "tsv" format explicitely. Update the doc to make it clear that the textual CSV, TSV and TXT formats pass the cursor back to the user through the Cursor HTTP header. (cherry picked from commit 3a8b00c) * Java8 fixes - replace Set#of(); - URLDecoder#decode() requires a string (vs a charset) as 2nd arg.
This PR adds the option to provide the desired character as the separator
for the CSV format (the default remains comma).
A set of characters are excluded though - like CR, LF,
"
- to avoidslipping onto the CSV-dialects slope. The tab is also forbidden, the
user needs to choose the
tsv
format explicitely.Closes #41634.