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

Version 2.0 #507

Merged
merged 2 commits into from
Mar 24, 2022
Merged

Version 2.0 #507

merged 2 commits into from
Mar 24, 2022

Conversation

vmmusings
Copy link
Member

@vmmusings vmmusings commented Mar 22, 2022

Description

This PR Covers the following items.

  • Version 2.0 bump
  • Gradle 7 Upgrade
  • JDK 8 removal
  • Handled removal of mapping types in SQL Plugin.

Issues Resolved

#303
#466
#452

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.

@vmmusings vmmusings marked this pull request as ready for review March 22, 2022 18:01
@vmmusings vmmusings requested a review from a team as a code owner March 22, 2022 18:01
@vmmusings vmmusings marked this pull request as draft March 22, 2022 18:01
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022

Codecov Report

Merging #507 (5801a1d) into main (2050899) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #507      +/-   ##
============================================
- Coverage     95.25%   95.24%   -0.01%     
+ Complexity     2746     2744       -2     
============================================
  Files           276      276              
  Lines          7416     7410       -6     
  Branches        544      537       -7     
============================================
- Hits           7064     7058       -6     
  Misses          298      298              
  Partials         54       54              
Flag Coverage Δ
query-workbench 62.91% <ø> (ø)
sql-engine 98.40% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...ch/sql/opensearch/client/OpenSearchNodeClient.java 100.00% <100.00%> (ø)
.../opensearch/sql/planner/physical/SortOperator.java 100.00% <0.00%> (ø)

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 2050899...5801a1d. Read the comment docs.

Signed-off-by: vamsi-amazon <[email protected]>
@vmmusings vmmusings marked this pull request as ready for review March 23, 2022 21:50
mavenCentral()
}

dependencies {
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this change is required by all plugin? if so, could you add related issue/pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are using a third party plugin to spawn and kill process in doctest module. The plugin is around 5 years old and there is no active development. As part of Gradle 7 upgrade spawn plugin is throwing errors. As part of fix, imported the plugin code to buildSrc and upgraded to Gradle 7.[The only way for upgrading unmaintained third party plugins]. I couldn't find another well maintained plugin to replace.

Copy link
Member Author

Choose a reason for hiding this comment

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

#303. Will add to the description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks. could we create an issue to tracking move this dependency later. ideally, we should not depend on it.

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.

Could you add some comments that why tests in integ-test/doctest are deleted. If it is for deprecate legacy not used test, I would prefer to have a separated PR .

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.

Please also help revert change introduce in #483. We could do it separately.


import org.gradle.api.tasks.TaskAction

class KillProcessTask extends DefaultSpawnTask {
Copy link
Member

Choose a reason for hiding this comment

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

what is this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

#507 (comment)

Commented here.

gradle/wrapper/gradle-wrapper.properties Show resolved Hide resolved
gradlew Show resolved Hide resolved
@vmmusings
Copy link
Member Author

Could you add some comments that why tests in integ-test/doctest are deleted. If it is for deprecate legacy not used test, I would prefer to have a separated PR .

@penghuo It was deleted after consulting with @dai-chen. The reason to remove them in this CR is when we are bumping to 2.0, these files are causing build issues with breaking changes from upstream libraries. I felt the better way to handle is to remove them in this CR itself.

Signed-off-by: vamsi-amazon <[email protected]>
@dai-chen
Copy link
Collaborator

Could you add some comments that why tests in integ-test/doctest are deleted. If it is for deprecate legacy not used test, I would prefer to have a separated PR .

@penghuo It was deleted after consulting with @dai-chen. The reason to remove them in this CR is when we are bumping to 2.0, these files are causing build issues with breaking changes from upstream libraries. I felt the better way to handle is to remove them in this CR itself.

yeah, the doctest package is Java code that generates the docs initially. It is not used any more and should be safe to delete.

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.

Thanks for the change!

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!

@vmmusings vmmusings merged commit 6771968 into opensearch-project:main Mar 24, 2022
Yury-Fridlyand added a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Oct 5, 2022
Yury-Fridlyand added a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Oct 5, 2022
Yury-Fridlyand added a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants