Skip to content
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

Well format the raw response when query parameter "pretty" enabled #2727

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

LantaoJin
Copy link
Member

Description

Well format the raw response when query parameter pretty enabled (/_plugins/_sql?format=raw&pretty=true or /_plugins/_sql?format=raw&pretty)

Example:

$>curl -k -H 'Content-Type: application/json' -XPOST 'http://localhost:9200/_plugins/_sql?format=raw' -d '{"query": "SELECT type, manufacturer, customer_last_name, day_of_week, total_quantity, currency, taxless_total_price FROM opensearch_dashboards_sample_data_ecommerce LIMIT 5"}'

returns

type|manufacturer|customer_last_name|day_of_week|total_quantity|currency|taxless_total_price
order|Elitelligence|Underwood|Monday|2|EUR|36.98
order|Champion Arts|Bailey|Sunday|2|EUR|53.98
order|Low Tide Media|Butler|Sunday|2|EUR|199.98
order|Primemaster|Chandler|Sunday|2|EUR|174.98
order|Elitelligence|Weber|Monday|2|EUR|80.98

$>curl -k -H 'Content-Type: application/json' -XPOST 'http://localhost:9200/_plugins/_sql?format=raw&pretty=true' -d '{"query": "SELECT type, manufacturer, customer_last_name, day_of_week, total_quantity, currency, taxless_total_price FROM opensearch_dashboards_sample_data_ecommerce LIMIT 5"}'

returns

type |manufacturer   |customer_last_name|day_of_week|total_quantity|currency|taxless_total_price
order|Elitelligence  |Underwood         |Monday     |2             |EUR     |36.98
order|Champion Arts  |Bailey            |Sunday     |2             |EUR     |53.98
order|Low Tide Media |Butler            |Sunday     |2             |EUR     |199.98
order|Primemaster    |Chandler          |Sunday     |2             |EUR     |174.98
order|Elitelligence  |Weber             |Monday     |2             |EUR     |80.98

Issues Resolved

#2718

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Lantao Jin <ltjin@amazon.com>
Signed-off-by: Lantao Jin <ltjin@amazon.com>
Comment on lines 63 to 65
private List<String> headers;
private List<List<String>> data;
private int[] maxWidths;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you refactor this class a little? I think this class was focus on sanitize only. After this, the class becomes more and more complex...

Copy link
Member

Choose a reason for hiding this comment

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

Although this is existing code, I don't think having inner static classes inside is a good pattern. While refactoring can we separte it out and have individual classes for Sanitizer and Prettifier if possible[Up to you how you refactor].

Copy link
Member Author

Choose a reason for hiding this comment

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

I've refactored for separating. Please take a look and provide feedbacks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add Javadoc for new class? In future, I think we can refactor base, sanitizer, prettier by decorator as needed, e.g. sanitize and then pretty.

Signed-off-by: Lantao Jin <ltjin@amazon.com>
Signed-off-by: Lantao Jin <ltjin@amazon.com>
Copy link
Collaborator

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

@penghuo penghuo merged commit aec4825 into opensearch-project:main Jul 9, 2024
14 of 21 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-2727-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 aec482577143a327a6dc94e073a0a6cec709a7ed
# Push it to GitHub
git push --set-upstream origin backport/backport-2727-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2727-to-2.x.

LantaoJin added a commit to LantaoJin/search-plugins-sql that referenced this pull request Jul 16, 2024
…pensearch-project#2727)

Signed-off-by: Lantao Jin <ltjin@amazon.com>
(cherry picked from commit aec4825)
penghuo pushed a commit that referenced this pull request Jul 18, 2024
…2727) (#2829)

Signed-off-by: Lantao Jin <ltjin@amazon.com>
(cherry picked from commit aec4825)
manasvinibs pushed a commit to manasvinibs/sql that referenced this pull request Aug 14, 2024
jzonthemtn pushed a commit to jzonthemtn/sql that referenced this pull request Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants