Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add CSV formatter in new engine #870

Merged
merged 21 commits into from
Dec 11, 2020

Conversation

chloe-zh
Copy link
Member

@chloe-zh chloe-zh commented Dec 1, 2020

Issue #, if available:
#148 #117 #100

Description of changes:

  • Added csv formatter CsvResponseFormatter in new engine. The formatter formats the QueryResult to csv format and by default it sanitizes the result by (1) inserting single-quote in front of header/data if it starts with special characters including +, -, = and @; (2) quoting the header/data if one or more commas (,) in the header/data. The sanitizing can be skipped by adding an sanitize param and set it to false value in the request.

  • User manual updates: https://github.com/chloe-zh/sql/blob/csv-formatter/docs/user/interfaces/protocol.rst

  • Added tests for the formatter; ignored a couple of legacy tests that are not applicable for the new engine

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chloe-zh chloe-zh self-assigned this Dec 1, 2020
@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #870 (84f6f84) into develop (0cb3240) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             develop     #870    +/-   ##
===========================================
  Coverage      99.85%   99.86%            
- Complexity      2148     2217    +69     
===========================================
  Files            216      225     +9     
  Lines           4850     5129   +279     
  Branches         320      336    +16     
===========================================
+ Hits            4843     5122   +279     
  Misses             5        5            
  Partials           2        2            
Impacted Files Coverage Δ Complexity Δ
...relasticsearch/sql/ppl/domain/PPLQueryRequest.java 100.00% <100.00%> (ø) 6.00 <4.00> (+3.00)
...protocol/response/format/CsvResponseFormatter.java 100.00% <100.00%> (ø) 4.00 <4.00> (?)
...h/sql/protocol/response/format/ErrorFormatter.java 100.00% <100.00%> (ø) 9.00 <9.00> (?)
...ticsearch/sql/protocol/response/format/Format.java 100.00% <100.00%> (ø) 4.00 <4.00> (?)
...rotocol/response/format/JsonResponseFormatter.java 100.00% <100.00%> (ø) 7.00 <5.00> (-1.00)
...relasticsearch/sql/sql/domain/SQLQueryRequest.java 100.00% <100.00%> (ø) 21.00 <11.00> (+8.00)
...ndistroforelasticsearch/sql/analysis/Analyzer.java 100.00% <0.00%> (ø) 43.00% <0.00%> (ø%)
...distroforelasticsearch/sql/data/type/ExprType.java 100.00% <0.00%> (ø) 7.00% <0.00%> (+1.00%)
...troforelasticsearch/sql/sql/parser/AstBuilder.java 100.00% <0.00%> (ø) 25.00% <0.00%> (+3.00%)
...roforelasticsearch/sql/data/type/ExprCoreType.java 100.00% <0.00%> (ø) 10.00% <0.00%> (+1.00%)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cb3240...84f6f84. Read the comment docs.

@chloe-zh chloe-zh marked this pull request as ready for review December 8, 2020 19:50
@chloe-zh chloe-zh requested review from dai-chen and penghuo December 8, 2020 19:51
@chloe-zh chloe-zh requested a review from harold-wang December 8, 2020 22:03
@dai-chen
Copy link
Member

dai-chen commented Dec 9, 2020

Could you add the following related issue to the description? I think we should be able to close them after this PR merged.

  1. CSV and JDBC export are 2 different execution paths #148
  2. JDBC and CSV format do not support column aliases #117
  3. Wrong order of returned fields when function is used in select list #100
  4. Improve raw formatter with more options #546

@chloe-zh
Copy link
Member Author

chloe-zh commented Dec 9, 2020

Could you add the following related issue to the description? I think we should be able to close them after this PR merged.

  1. CSV and JDBC export are 2 different execution paths #148
  2. JDBC and CSV format do not support column aliases #117
  3. Wrong order of returned fields when function is used in select list #100
  4. Improve raw formatter with more options #546

Added but the last issue #546 because this PR didn't enable custom separators yet

@dai-chen
Copy link
Member

dai-chen commented Dec 9, 2020

Could you add the following related issue to the description? I think we should be able to close them after this PR merged.

  1. CSV and JDBC export are 2 different execution paths #148
  2. JDBC and CSV format do not support column aliases #117
  3. Wrong order of returned fields when function is used in select list #100
  4. Improve raw formatter with more options #546

Added but the last issue #546 because this PR didn't enable custom separators yet

Got it, you can close that one once this merged. Thanks!

Copy link
Member

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes!

@chloe-zh chloe-zh merged commit daee92e into opendistro-for-elasticsearch:develop Dec 11, 2020
harold-wang pushed a commit to harold-wang/sql that referenced this pull request Dec 11, 2020
* support csv format in new engine

* skipped some IT cases that are not applicable in new engine

* udpate

* added escape option

* added test for Format

* update

* update

* added IT for ppl

* added IT for ppl

* added IT for ppl

* added license header

* added test for sql

* added example in protocol doc

* addressed comments

* added responseParams override method to sql and ppl stats actions

* added unit test cases

* update

* addressed comment

* addressed comments
penghuo pushed a commit that referenced this pull request Dec 15, 2020
* support csv format in new engine

* skipped some IT cases that are not applicable in new engine

* udpate

* added escape option

* added test for Format

* update

* update

* added IT for ppl

* added IT for ppl

* added IT for ppl

* added license header

* added test for sql

* added example in protocol doc

* addressed comments

* added responseParams override method to sql and ppl stats actions

* added unit test cases

* update

* addressed comment

* addressed comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants