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: replace Float/Double NaN with null #98698

Closed
costin opened this issue Aug 21, 2023 · 8 comments
Closed

ESQL: replace Float/Double NaN with null #98698

costin opened this issue Aug 21, 2023 · 8 comments
Assignees
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@costin
Copy link
Member

costin commented Aug 21, 2023

Description

Floating number (Float and Double) provide a special, out of band value - NaN, Not-a-Number.

This occurs typically when dividing a number by zero; integer numbers don't have one and instead thrown an error.
To make things consistent, I propose we treat and convert NaN internally to null so it never reaches the user.
Several arguments to support this:

  • JSON doesn't allow NaN (nor -Inf or +Inf for that matter)
  • ES doesn't allow ingestion of non-finite double or float (regardless of whether JSON is used or not) - see NumberFieldMapper for floats and doubles.
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@not-napoleon
Copy link
Member

As I said at the meeting yesterday, I think it's less important which option we pick, so long as we pick one and do it consistently in all functions.

This has an advantage that it's easier to test - if nothing should return NaN, we can just add an assert to the unit tests that we never get back a NaN, rather than having to think about each case and if it should be NaN or null.

Would we also collapse the infinities into null?

@not-napoleon not-napoleon self-assigned this Sep 12, 2023
not-napoleon added a commit that referenced this issue Sep 12, 2023
Relates to #98698

Add asserts that we aren't returning NaN or Infinite values from scalar functions. I also fixed the functions that already had test cases returning NaN/Infinite values. Note that this doesn't fix all our scalar functions, as some may just not have test cases that hit these outputs regularly, which is why this PR doesn't close the above issue.
@mattc58 mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023
@bpintea
Copy link
Contributor

bpintea commented Dec 8, 2023

Just noting this for future lookups:

Would we also collapse the infinities into null?

We practically have:

private static double validateAsDouble(double base, double exponent) {
double result = Math.pow(base, exponent);
if (Double.isNaN(result) || Double.isInfinite(result)) {
throw new ArithmeticException("invalid result when computing pow");
}
return result;
}

@not-napoleon
Copy link
Member

@bpintea Yes, that's correct. We decided that NaN, and both infinities should be rendered as null. I don't recall all the reasoning behind this, but one factor was that the infinite values aren't representable in JSON

@wchaparro wchaparro added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 2, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@elasticsearchmachine elasticsearchmachine removed the Team:QL (Deprecated) Meta label for query languages team label Jan 2, 2024
@alex-spies alex-spies self-assigned this Jan 17, 2024
@bpintea bpintea added >bug and removed >enhancement labels Feb 8, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@nik9000
Copy link
Member

nik9000 commented Jul 1, 2024

Save for a few bugs, I believe we've done this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants