ESQL: emit warnings from single-value functions processing multi-values #102417

bpintea · 2023-11-21T14:26:45Z

When encountering a multi-value, a single-value function (i.e. all non-mv_xxx()) returns a null. This behaviour is opaque to the user. This PR adds the functionality for these functions to emit a Warning header, so the user is informed about the cause for the nulls.

Within testing, there are some differences between the emulated CSV-based tests (TestPhysical*) and the REST CSV-tests and thus the exact messages in the warnings:

The REST ones can push operations to Lucene; when this happens, a query containing a negation, not <predicate>, can be translated to a must_not query, that will include the not in the Source. But outside of Lucene, the execution would consider the predicate first, then the negation. So when the predicate contains a SV function, only this part's Source will show up in the warning.
When pushed to Lucene, a query is wrapped within the SingleValueQuery. This emits now warnings when encountering MVs (and returning no match). However, this only happens once the query that it wraps returns something itself. Comparatively, the TestPhysical* filters will issue a warning for every encountered MV (irrespective of sigle values within the MV matching or not).

To differentiate between the slightly differing values of the warnings, one can now append the #[Emulated: prefix to a warning, followed by the value of the warning for the emulated checks, then a corresponding ].
Example: warning:Line 1:24: evaluation of [not(salary_change < 1)] failed, treating result as null. Only first 20 failures recorded.#[Emulated:Line 1:28: evaluation of [salary_change < 1] failed, treating result as null. Only first 20 failures recorded.]

Closes #98743.

This adds Warning headers when scalar singlevalue functions enounter multivalues (and emits a `null`).

This adds warning generation to SingleValueQuery. Two issues with this approch, however: - the source is serialized with the text, the EsqlConfiguration isn't made available. - the translation of queries for Lucene differs from the exec engine, in respect to not(): `must_not` query "contains" the `not` and includes it in its source, while in the exec engine its execution happens in two separate steps (so the warning is added to the predicate the not() negates).

Allow slightly different warnings for Rest and CSV tests.

Currently, this was done on a `"values_count()" != 1`, which is wrong, since it has to differentiate from a 0 count.

elasticsearchmachine · 2023-11-21T14:27:10Z

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

elasticsearchmachine · 2023-11-21T14:27:46Z

Hi @bpintea, I've updated the changelog YAML for you.

Skip bwc tests on pre 8.12 versions. Also, fix doc test.

elasticsearchmachine · 2023-11-21T18:38:24Z

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

elasticsearchmachine · 2023-11-21T18:38:25Z

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

ChrisHegarty

Nice! I just left a comment about the serial compatibility.

ChrisHegarty · 2023-11-23T09:54:21Z

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueQuery.java

        }

        Builder(StreamInput in) throws IOException {
            super(in);
            this.next = in.readNamedWriteable(QueryBuilder.class);
            this.field = in.readString();
            this.stats = new Stats();
+            this.source = readSource(in);


Do we need to bump the transport version for this change?

Actually, where do we serialise this?

Do we need to bump the transport version for this change?

Good point, we do. I've added a new transport version.

Actually, where do we serialise this?

The doWriteTo() method below updates the serialisation based on the transport version.

alex-spies

Great! I only have minor remarks except for what Chris already pointed out.

Like Chris, I think we'll need to bump transport version since we're now sending the source along with single value queries.

Do we need to account for this in other ways as well? I guess if we have a transport connection to 8.11 nodes, we could just not send the source to avoid breaking bwc?

x-pack/plugin/ql/test-fixtures/src/main/java/org/elasticsearch/xpack/ql/CsvSpecReader.java

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/querydsl/query/SingleValueQuery.java

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

astefan

LGTM

bpintea · 2023-12-04T14:37:10Z

@elasticsearchmachine run elasticsearch-ci/docs

costin · 2023-12-05T03:12:43Z

This is a big usability improvement - let's get this in for 8.12.
From my side, I wonder if we could remove the exception classname and just rely on the error message itself as that's what the user cares about.
Talking about the message, how about pointing the user either to the docs (chapter/section) or specifying the workaround - use mv_expand.

bpintea · 2023-12-05T09:22:25Z

@elasticsearchmachine run elasticsearch-ci/part-3 elasticsearch-ci

bpintea · 2023-12-05T13:36:29Z

I've merged this as-is, since it bumps the TransportVersions, which is popular before FF.

From my side, I wonder if we could remove the exception classname and just rely on the error message itself as that's what the user cares about.

We could, yes. The only two concerns I'd have is that:

it would be slightly misaligned with the general failure response;
it might be misleading in some of the failures, like number parsing: java.lang.NumberFormatException: For input string: "xx" -- the way the warning is generated is reused in SingleValueQuery.

Talking about the message, how about pointing the user either to the docs (chapter/section) or specifying the workaround - use mv_expand.

I'll update the message.

bpintea added 8 commits October 4, 2023 14:15

Add Warning headers when SV functions encounter MV

0f576e1

This adds Warning headers when scalar singlevalue functions enounter multivalues (and emits a `null`).

Allow different warnings for Rest and CSV tests

1f896fa

Allow slightly different warnings for Rest and CSV tests.

Merge branch 'main' into esql/warn_sv_func_on_mv2

ed90baf

Merge branch 'main' into esql/warn_sv_func_on_mv2

7993278

Merge branch 'main' into esql/warn_sv_func_on_mv2

715114e

Update unit tests to SV-funcs warnings

e774a85

Fix generation of warnings in the evaluators

9705e9b

Currently, this was done on a `"values_count()" != 1`, which is wrong, since it has to differentiate from a 0 count.

bpintea added >feature :Analytics/ES|QL AKA ESQL v8.12.0 labels Nov 21, 2023

bpintea added 2 commits November 21, 2023 15:27

Update docs/changelog/102417.yaml

d921a23

Update docs/changelog/102417.yaml

f73f890

bpintea added 4 commits November 21, 2023 15:35

Spotless

66042ed

Spotless

b5d8308

Skip some bwc tests

f2451a6

Skip bwc tests on pre 8.12 versions. Also, fix doc test.

Merge branch 'main' into esql/warn_sv_func_on_mv2

e353b47

bpintea marked this pull request as ready for review November 21, 2023 18:38

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Nov 21, 2023

ChrisHegarty reviewed Nov 23, 2023

View reviewed changes

alex-spies reviewed Nov 23, 2023

View reviewed changes

bpintea added 2 commits November 23, 2023 23:41

Address review comments

4b65f5d

Merge branch 'main' into esql/warn_sv_func_on_mv2

e14b485

alex-spies approved these changes Nov 24, 2023

View reviewed changes

astefan approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'main' into esql/warn_sv_func_on_mv2

3ea8c03

bpintea added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 4, 2023

bpintea added 2 commits December 4, 2023 14:48

Update geo comparasion

5b1a6b7

Merge branch 'main' into esql/warn_sv_func_on_mv2

2a6368f

Merge branch 'main' into esql/warn_sv_func_on_mv2

bf388c5

Merge branch 'main' into esql/warn_sv_func_on_mv2

7992f8a

elasticsearchmachine merged commit 8d0551e into elastic:main Dec 5, 2023
15 checks passed

bpintea deleted the esql/warn_sv_func_on_mv2 branch December 5, 2023 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: emit warnings from single-value functions processing multi-values #102417

ESQL: emit warnings from single-value functions processing multi-values #102417

bpintea commented Nov 21, 2023 •

edited

Loading

elasticsearchmachine commented Nov 21, 2023

elasticsearchmachine commented Nov 21, 2023

elasticsearchmachine commented Nov 21, 2023

elasticsearchmachine commented Nov 21, 2023

ChrisHegarty left a comment

ChrisHegarty Nov 23, 2023

ChrisHegarty Nov 23, 2023

bpintea Nov 24, 2023

alex-spies left a comment

astefan left a comment

bpintea commented Dec 4, 2023

costin commented Dec 5, 2023

bpintea commented Dec 5, 2023

bpintea commented Dec 5, 2023

ESQL: emit warnings from single-value functions processing multi-values #102417

ESQL: emit warnings from single-value functions processing multi-values #102417

Conversation

bpintea commented Nov 21, 2023 • edited Loading

elasticsearchmachine commented Nov 21, 2023

elasticsearchmachine commented Nov 21, 2023

elasticsearchmachine commented Nov 21, 2023

elasticsearchmachine commented Nov 21, 2023

ChrisHegarty left a comment

Choose a reason for hiding this comment

ChrisHegarty Nov 23, 2023

Choose a reason for hiding this comment

ChrisHegarty Nov 23, 2023

Choose a reason for hiding this comment

bpintea Nov 24, 2023

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

bpintea commented Dec 4, 2023

costin commented Dec 5, 2023

bpintea commented Dec 5, 2023

bpintea commented Dec 5, 2023

bpintea commented Nov 21, 2023 •

edited

Loading