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.

Sign up for GitHub

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

Jump to bottom

SQL pagination should work with the pretty parameter #2759

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

SQL pagination should work with the pretty parameter #2759

wants to merge 5 commits into from

Conversation

LantaoJin
Copy link
Member

@LantaoJin LantaoJin commented Jun 17, 2024 •
edited
Loading

Description

  1. Refactoring isSupported() improves readability by clearly defining each condition and simplifying the overall logic.
  2. PRETTY (plus FORMAT) parameters should be treated as supported in validating Cursor request.

Issues Resolved

resolves #2460

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.

@LantaoJin
SQL pagination should work with the pretty parameter
d851cdc 
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin LantaoJin changed the title SQL pagination should work with the pretty parameter SQL pagination should work with the pretty parameter Jun 17, 2024
@LantaoJin
rename
330a149 
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin
fix spotless
bb29410 
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@codecov Codecov
Copy link

codecov bot commented Jun 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.14%. Comparing base (7b40c2c) to head (bb29410).
Report is 5 commits behind head on main.

❗ Current head bb29410 differs from pull request most recent head 6607f35

Please upload reports for the commit 6607f35 to get more accurate results.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #2759   +/-   ##
=========================================
  Coverage     92.14%   92.14%           
  Complexity     4960     4960           
=========================================
  Files           496      496           
  Lines         14341    14345    +4     
  Branches        942      945    +3     
=========================================
+ Hits          13214    13218    +4     
  Misses         1091     1091           
  Partials         36       36
Flag Coverage Δ
sql-engine 92.14% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

penghuo
penghuo previously approved these changes Jun 20, 2024
View reviewed changes
dai-chen
dai-chen previously approved these changes Jun 24, 2024
View reviewed changes
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 fix!

vamsi-amazon
vamsi-amazon reviewed Jun 24, 2024
View reviewed changes
sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java Outdated
boolean hasQuery = query != null;
boolean hasContent = jsonContent != null && !jsonContent.isEmpty();

Predicate<String> supportedParams = List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains;
Copy link
Member

Choose a reason for hiding this comment

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

[NIT]won't Set.of would be more appropriate here?

@vamsi-amazon
Copy link
Member

Any ITs with pretty parameter? Is pretty parameter working?

@vamsi-amazon vamsi-amazon added the bug Something isn't working label Jun 24, 2024
@LantaoJin
Copy link
Member Author

LantaoJin commented Jun 25, 2024 •
edited
Loading

Any ITs with pretty parameter? Is pretty parameter working?

Currently we don't have a dedicated IT for testing pretty parameter. But we added some tests in CursorIT with c25aca6#diff-478b4af66a28fbcde418ed236aac5dde3903d573bfb6c016e3a43189e028c0e2R245

@LantaoJin
address comments
c25aca6 
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin LantaoJin dismissed stale reviews from dai-chen and penghuo via c25aca6 June 25, 2024 06:56
@LantaoJin
Copy link
Member Author

Hi @vamsi-amazon @penghuo @dai-chen, could you review and approve this PR again since I addressed comments.

@LantaoJin
apply spotless
6607f35 
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin
Copy link
Member Author

None of build issues are related to this code changes. But anyone knows why our CI has lots of failures in recent one or two weeks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vamsi-amazon vamsi-amazon vamsi-amazon left review comments

@penghuo penghuo penghuo left review comments

@dai-chen dai-chen dai-chen left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@rupal-bq rupal-bq Awaiting requested review from rupal-bq rupal-bq is a code owner

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand Yury-Fridlyand is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey forestmvey is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG is a code owner

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@LantaoJin LantaoJin

Labels
backport 2.x bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] SQL pagination doesn't work in Dev tools
4 participants
@LantaoJin @vamsi-amazon @penghuo @dai-chen