Improve MvPSeriesWeightedSum edge case and add more tests by machadoum · Pull Request #111552 · elastic/elasticsearch

machadoum · 2024-08-02T12:45:35Z

Update MvPSeriesWeightedSum function to return null + warnings instead of Infinite values.
Add extra tests to MvPSeriesWeightedSum for edge case scenarios.

I ran the tests 100 times to ensure they didn't break due to random values.

elasticsearchmachine · 2024-08-05T08:22:48Z

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

elasticsearchmachine · 2024-08-05T08:22:49Z

Hi @machadoum, I've created a changelog YAML for you.

...org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java

ivancea · 2024-08-05T09:54:00Z

...lugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/EvaluatorImplementer.java

                    args.add(new BlockProcessFunctionArg(type, name));
                    continue;
                }
+                if (type.equals(WARNINGS)) {


I'm not sure these changes are required after using warnExceptions (The other comment)?

I believe they are because the other comment doesn't work. There might be another way, but I didn't see one at the time.

ivancea · 2024-08-05T09:58:14Z

...org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java

+        if (p.dataType() == NULL) {
+            return resolution;
+        }


TypeResolutions.isType() already returns a "resolved" for nulls. So I believe this check isn't really doing anything

Ah! that one may be the case.

I'll have a look.

ivancea · 2024-08-05T10:01:13Z

...lasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSumTests.java

+        cases = randomizeBytesRefsOffset(cases);
+        cases = anyNullIsNull(
+            cases,
+            (nullPosition, nullValueDataType, original) -> nullValueDataType == DataType.NULL ? DataType.NULL : original.expectedType(),
+            (nullPosition, nullData, original) -> {
+                if (nullData.isForceLiteral()) {
+                    return equalTo("LiteralsEvaluator[lit=null]");
+                }
+                return nullData.type() == DataType.NULL ? equalTo("LiteralsEvaluator[lit=null]") : original;
+            }
+        );
+        cases = errorsForCasesWithoutExamples(cases, (valid, position) -> "double");
+


I wonder if it would be easier to just not require a literal/foldable as the second parameter? We really don't need a literal there, everything should run as smoothly, and maybe we avoid having weird cases with nulls.

Saying this, as I find it weird that we need such extra testing logics for this function. Two birds with one stone (probably?)

I had talked with @machadoum about forcing a literal here because it's the way they run it and they can use the forced-literal-ness in the executor. We can always change it later if folks need it. I agree the force-literal-ness makes this function quite a bit more....

I feel like we're adding complexity, just to reduce the options. It's done now, so we can go with it, but I would personally remove the literal forcing before doing much more things

…to siem-ea-9521-improve-test

nik9000

@ivancea I pushed an update. There's a fun thing around null that kind of bothers me. I left a TODO around it though so we can get the extra testing in....

ivancea

I think it's fine now, but I'd do some continuations on it that should clean some code

ivancea · 2024-08-06T08:30:21Z

...org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java

+        if (p.dataType() == NULL) {
+            // If the type is `null` this parameter doesn't have to be foldable. It's effectively foldable anyway.
+            // TODO figure out if the tests are wrong here, or if null is really different from foldable null
+            return resolution;
+        }


FYI: if it's null, FoldNull rule will convert the full expression into a literal null. And we explicitly call that rule in the tests. See:

elasticsearch/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/AbstractScalarFunctionTestCase.java

Line 93 in c5da257

expression = new FoldNull().rule(expression);

FoldNull checks if it's either NULL or is foldable and fold() == null. isFoldable() just checks foldability.

Btw, before touching anymore this method, I really think we should make it work with non-constants. Looks like we're "repeatedly" bumping into problems other functions don't have just because of it :')

ivancea · 2024-08-06T08:33:09Z

...org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPSeriesWeightedSum.java


    @Override
    public DataType dataType() {
+        if (p.dataType() == NULL) {


Do we really need this? Yes, it will return a NULL. But it's because of a planner rule, not because this functions can return nulls in any way (If that makes sense?).

This is not how other functions work, so I feel like we're overcomplicating things here

nik9000 · 2024-08-06T16:24:36Z

I'm going to merge this as is so we can add a few more test cases and move on. I do think this deserves more love though.

* upstream/main: (132 commits) Fix compile after several merges Update docs with new behavior on skip conditions (elastic#111640) Skip on any instance of node or version features being present (elastic#111268) Skip on any node capability being present (elastic#111585) [DOCS] Publishes Anthropic inference service docs. (elastic#111619) Introduce `ChunkedZipResponse` (elastic#109820) [Gradle] fix esql compile cacheability (elastic#111651) Mute org.elasticsearch.datastreams.logsdb.qa.StandardVersusLogsIndexModeChallengeRestIT testTermsQuery elastic#111666 Mute org.elasticsearch.datastreams.logsdb.qa.StandardVersusLogsIndexModeChallengeRestIT testMatchAllQuery elastic#111664 Mute org.elasticsearch.xpack.esql.analysis.VerifierTests testMatchCommand elastic#111661 Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommandWithMultipleMatches {default} elastic#111660 Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommand {default} elastic#111659 Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommandWithWhereClause {default} elastic#111658 LogsDB qa tests - add specific matcher for source (elastic#111568) ESQL: Move `randomLiteral` (elastic#111647) [ESQL] Clean up UNSUPPORTED type blocks (elastic#111648) ESQL: Remove the `NESTED` DataType (elastic#111495) ESQL: Move more out of esql-core (elastic#111604) Improve MvPSeriesWeightedSum edge case and add more tests (elastic#111552) Add link to flood-stage watermark exception message (elastic#111315) ... # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

* Update `MvPSeriesWeightedSum` function to return `null` + warnings instead of Infinite values. * Add extra tests to `MvPSeriesWeightedSum` for edge case scenarios. I ran the tests 100 times to ensure they didn't break due to random values.

…1552) * Update `MvPSeriesWeightedSum` function to return `null` + warnings instead of Infinite values. * Add extra tests to `MvPSeriesWeightedSum` for edge case scenarios. I ran the tests 100 times to ensure they didn't break due to random values.

elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.16.0 labels Aug 2, 2024

wip

048932f

machadoum force-pushed the siem-ea-9521-improve-test branch from 55957bf to 048932f Compare August 2, 2024 12:46

nik9000 added :Analytics/ES|QL AKA ESQL and removed external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 2, 2024

More fixes

c4518b0

machadoum self-assigned this Aug 5, 2024

machadoum added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) external-contributor Pull request authored by a developer outside the Elasticsearch team >enhancement labels Aug 5, 2024

machadoum marked this pull request as ready for review August 5, 2024 08:22

machadoum requested review from ivancea and nik9000 August 5, 2024 08:22

Update docs/changelog/111552.yaml

669c489

machadoum changed the title ~~Siem ea 9521 improve test~~ Improve MvPSeriesWeightedSum edge case and add more tests Aug 5, 2024

ivancea reviewed Aug 5, 2024

View reviewed changes

nik9000 added 4 commits August 5, 2024 12:26

Yet more

5b40ceb

Merge remote-tracking branch 'machadoum/siem-ea-9521-improve-test' in…

eb1fbb7

…to siem-ea-9521-improve-test

Merge branch 'main' into siem-ea-9521-improve-test

bfb3669

Merge

480229f

nik9000 reviewed Aug 5, 2024

View reviewed changes

ivancea approved these changes Aug 6, 2024

View reviewed changes

nik9000 removed the external-contributor Pull request authored by a developer outside the Elasticsearch team label Aug 6, 2024

nik9000 merged commit b803c2a into elastic:main Aug 6, 2024

Conversation

machadoum commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 5, 2024

Uh oh!

elasticsearchmachine commented Aug 5, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

ivancea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Aug 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

machadoum commented Aug 2, 2024 •

edited

Loading