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

Main test: Update query returned records limit #30

Merged
merged 6 commits into from
Sep 25, 2024

Conversation

Swiddis
Copy link
Collaborator

@Swiddis Swiddis commented Sep 25, 2024

Description

When running in CI via tox, the returned limit seems to be 10000, while locally it's 200 by default. This means that one of the two test sets is always failing depending on what this number is. This PR manually specifies the limit to remove this inconsistency.

Also, side note: if you have the old cli_helpers[styles]=1.3 locally after the 2.3 dependency was merged in main, formatting tests will start behaving inconsistently locally vs on CI. Pulling the new dep from main will fix it.

Issues Resolved

N/A

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: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis
Copy link
Collaborator Author

Swiddis commented Sep 25, 2024

Found the root cause: opensearch-project/sql#2860 changes the limit from 200 to 10000 as of 2.17. This means you get different behavior depending on which version you run. I added a LIMIT clause to enforce consistency.

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
@Swiddis Swiddis merged commit d7f4041 into opensearch-project:main Sep 25, 2024
5 checks passed
Copy link
Contributor

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

@Swiddis should this backport to 2.x?

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.

3 participants