Skip to content

[v3.0.0] Deprecate OpenSearch DSL format #3367

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
Merged
merged 21 commits into from
Mar 10, 2025
Merged

[v3.0.0] Deprecate OpenSearch DSL format #3367

merged 21 commits into from
Mar 10, 2025

Conversation

@seankao-az
Copy link
Collaborator

@seankao-az seankao-az commented Feb 28, 2025 •
edited
Loading

Description

Removes the OpenSearch DSL response format.

This PR focus only on deprecating the format, and will silent the affected legacy test cases. #3372 is created to track the task to decide which of the legacy tests are worth keeping and what to do with them.

Documentation website changes required:

  • response_formats.md
  • limitation.md
  • sql-ppl-api.md

possibly more

Related Issues

Resolves #3280

Check List

- [ ] New functionality includes testing.

  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.

- [ ] API changes companion pull request created.

  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@seankao31
mark places to change
bd434ec 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
remove dsl format from doc
613da7c 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
remove Format.JSON
43fe0fa 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
fix test case wip
e3157aa 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az seankao-az changed the title Deprecate OpenSearch DSL format [v3.0.0] Deprecate OpenSearch DSL format Feb 28, 2025
@seankao31
Revert "fix test case wip"
85e2566 
This reverts commit e3157aa.

Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
ignore legacy test using json response format
5f0ac18 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
spotlessApply
3cbaf82 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
remove json param check in executeRequest
c74b96b 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
fix getScriptFieldFromQuery can't get requestBuilder from QueryPlan
48d512a 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
spotlessApply
d0c1ef1 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
ignore test using json
82cfce0 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
fix test case
5f38dd5 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
ignore legacy tests affected
1cdeccd 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
fix json call
3b50834 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
remove patch file
bf12b0f 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
comment
681554f 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az seankao-az mentioned this pull request Mar 3, 2025
Open
@seankao31
remove unneeded file
e449c44 
Signed-off-by: Sean Kao <seankao@amazon.com>
seankao-az
seankao-az commented Mar 3, 2025
View reviewed changes
legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java Outdated
sqlExpr.accept(new TermFieldRewriter());
// migrate aggregation to query planner framework.
if (shouldMigrateToQueryPlan(sqlExpr, request.getFormat())) {
if (!bypassMigrateToQueryPlan && shouldMigrateToQueryPlan(sqlExpr)) {
Copy link
Collaborator Author

@seankao-az seankao-az Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Previously some test cases use Format.JSON to bypass this check and get QueryAction for SELECT (either AggregationQueryAction or DefaultQueryAction). After removing Format.JSON, those test case wouldn't work without this flag.

@seankao-az seankao-az marked this pull request as ready for review March 3, 2025 19:41
@seankao31
delete unused class
b10d148 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az seankao-az self-assigned this Mar 3, 2025
dai-chen
dai-chen reviewed Mar 4, 2025
View reviewed changes
legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java
Comment on lines -197 to -199
if (format == Format.JSON) {
return false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now there is no JSON format, I'm thinking should we ignore/fix the failed IT instead of adding a flag bypassMigrateToQueryPlan?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

main problem trying to solve with this flag is CheckScriptContents.getScriptFieldFromQuery needs access to a SearchRequestBuilder in QueryAction, which I think QueryPlanQueryAction doesn't expose. Will double check if there's another way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for the tests that used this method, it's a simple fix to change the query to not have any aggregation. Doing so will avoid migrating to QueryPlanner as well.

Those are UTs testing the string operators, and the GROUP BY and ORDER BY aren't used anyway, so it should be safe to remove them.

seankao31 added 3 commits March 4, 2025 11:45
@seankao31
revert change in prod code for test purpose; fix test
d43a5c0 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
re-evaluate affected tests
06bea4f 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao31
spotlessApply
085647f 
Signed-off-by: Sean Kao <seankao@amazon.com>
@seankao-az seankao-az requested a review from dai-chen March 4, 2025 21:43
penghuo
penghuo reviewed Mar 5, 2025
View reviewed changes
legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java Outdated
return create(client, sql, false);
}

public static QueryAction create(Client client, String sql, boolean bypassMigrateToQueryPlan)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function only used in UT? if so, add a tag/java-doc on this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i removed this in latest commit. It was an attempt to fix some test cases, but it turns out changing the test cases is easier.

@seankao-az seankao-az requested a review from penghuo March 6, 2025 17:31
dai-chen
dai-chen approved these changes Mar 6, 2025
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 changes!

penghuo
penghuo approved these changes Mar 7, 2025
View reviewed changes
Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thx!

@noCharger noCharger merged commit f750bc6 into opensearch-project:main Mar 10, 2025
22 checks passed
@xinyual xinyual mentioned this pull request Jun 11, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@penghuo penghuo penghuo approved these changes

@noCharger noCharger noCharger approved these changes

@dai-chen dai-chen dai-chen approved these changes

@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

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

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis 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

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

Assignees

@seankao-az seankao-az

Labels

breaking v3.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deprecated OpenSearch DSL format.

5 participants

@seankao-az @penghuo @noCharger @dai-chen @seankao31