Esql eager nans to nulls #98899
Esql eager nans to nulls #98899
Conversation
Conflicts: x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java
elasticsearchmachine
added
needs:triage
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Hi @not-napoleon, I've created a changelog YAML for you. |
elasticsearchmachine
removed
the
needs:triage
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
There was a problem hiding this comment.
Generally LGTM, I just wonder if we really want to get rid of the infinities as well. (The discussion on the corresponding issue #98698 had no further comments.)
| @@ -210,48 +210,6 @@ salary:integer | s:long | |||
| 73851 | 1330201 | |||
| ; | |||
|
|
|||
| isFiniteFalse | |||
| row d = 1.0 | eval s = is_finite(d/0); | |||
There was a problem hiding this comment.
question: I see that there were no more comments on your question whether to treat infinities as null, and here we seem to pick the Infinity -> null option.
I may be a bit out of the loop here :) if the discussion proceeded elsewhere than #98698, I'm curious why we want to treat also the infinities as null?
(For what it's worth, I think the infinities are semantically different from NaNs and it's less natural to treat +-Infinity as null, whereas NaN and null have very close meaning to begin with. However, for practical matters, I can imagine that in the end it might not make a difference for the end user and treating all the non-finite double values as null might simplify everything.)
| @@ -266,6 +267,7 @@ public final void testSimple() { | |||
| // TODO should we convert unsigned_long into BigDecimal so it's easier to assert? | |||
| Object result = toJavaObject(evaluator(expression).get().eval(row(testCase.getDataValues())), 0); | |||
| assertThat(result, testCase.getMatcher()); | |||
| assertThat(result, not(equalTo(Double.NaN))); | |||
There was a problem hiding this comment.
question: should we also assert that the result is neither of the infinities?
| if (rhs == 0) { | ||
| throw new ArithmeticException("Division by zero"); | ||
| } | ||
| return lhs / rhs; |
There was a problem hiding this comment.
I think this does not cover all the edge cases if we care about infinities. Dividing something big by something small can still end up infinite.
Maybe in this case it's easier to just perform lhs/rhs and throw an ArithmeticException if the result is not finite?
| .withWarning("java.lang.ArithmeticException: Division by zero") | ||
| ), | ||
| new TestCaseSupplier( | ||
| "1d / 0d", |
There was a problem hiding this comment.
suggestion: I'd add two tests, one for dividing a number close to Double.MAX_VALUE by something close to Double.MIN_VALUE or Double.MIN_NORMAL, and another test for the negative case.
Conflicts: x-pack/plugin/esql/qa/testFixtures/src/main/resources/show.csv-spec x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractFunctionTestCase.java
| arg = randomDouble(); | ||
| } else { | ||
| arg = 1 / randomDouble(); | ||
| } |
There was a problem hiding this comment.
In #98890 I've introduced something to emit cases for both big and little values. Just makes two cases every time.
|
We've moved the test framework in a different direction, and this PR is no longer appropriate. |
Resolve #98698
Turn NaNs and infinite results into nulls, as early as possible.