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

Esql warnings in tests #98699

Merged
merged 4 commits into from
Aug 22, 2023
Merged

Conversation

not-napoleon
Copy link
Member

@not-napoleon not-napoleon commented Aug 21, 2023

This adds an ability to verify warnings in specific test cases. I added a few overflow tests to pow as an example. PR also includes other test, and wiring more pow examples into the parameterized framework.

@elasticsearchmachine elasticsearchmachine added v8.11.0 needs:triage Requires assignment of a team area label labels Aug 21, 2023
}),
new TestCaseSupplier("long-double overflow", () -> {
long base = 4339622345450989181L; // Not exactly representable as a double
long expected = 4339622345450989056L;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a pretty concerning behavior. For longs > 2^52, which are not exactly representable as doubles, x^1 != x. I'm not sure what we should do about this.

Honestly, I think there's a strong argument that pow should always return a double (which is what Java does), since most powers of large longs will overflow anyway.

@not-napoleon not-napoleon added >non-issue >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL labels Aug 22, 2023
@elasticsearchmachine elasticsearchmachine added Team:QL (Deprecated) Meta label for query languages team and removed needs:triage Requires assignment of a team area label labels Aug 22, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

@not-napoleon not-napoleon requested a review from nik9000 August 22, 2023 16:37
this.expectedWarnings = null;
}

public TestCase(
Copy link
Member

Choose a reason for hiding this comment

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

This maybe should be private just so it's normal to build it without the warnings and add, I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Like, I imagine it'd be nice if there was one "normal" way to do it.

@not-napoleon not-napoleon merged commit 543c125 into elastic:main Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >non-issue Team:QL (Deprecated) Meta label for query languages team >test Issues or PRs that are addressing/adding tests v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants