Fix incorrect nullify with unmapped fields#142300
Fix incorrect nullify with unmapped fields#142300idegtiarenko merged 20 commits intoelastic:mainfrom
Conversation
...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @idegtiarenko, I've created a changelog YAML for you. |
alex-spies
left a comment
There was a problem hiding this comment.
Heya, this fixes #141870, right?
Can we maybe add a test suite that runs any spec tests that don't have a SETting and we add SET unmapped_fields="nullify" to it?
That would be a nice invariant that future tests also mustn't break this setting.
|
Hi @idegtiarenko, I've updated the changelog YAML for you. |
# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java
...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java
Outdated
Show resolved
Hide resolved
# Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/EsqlSpecTestCase.java
...ingle-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/EsqlSpecIT.java
Show resolved
Hide resolved
| // TODO we should implement more generic randomization for SET parameters | ||
| return randomBoolean() | ||
| && testCase.expectedWarnings().isEmpty() // avoid shifting warnings positions in source query | ||
| && testCase.expectedWarningsRegex().isEmpty() // regexp might also contain line/position | ||
| && query.startsWith("SET") == false // avoid conflicts with provided settings | ||
| ? "SET unmapped_fields=\"nullify\"; " + query | ||
| : query; |
There was a problem hiding this comment.
Maybe this should be the default randomization, rather than the single node specific one? What do you think?
There was a problem hiding this comment.
As discussed, this randomization only works if all nodes running the test support the required capabilities. This is generally only true for the single and multi-node tests, not the mixed cluster tests and ccq bwc tests.
Let's also randomize for our multi-node tests, though?
alex-spies
left a comment
There was a problem hiding this comment.
Super nice fix, thanks @idegtiarenko ! LGTM, only minor comments. Please address at your own discretion and 
, or let me know if you're out of capacity and I'll take this and foster it into main.
...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/rules/ResolveUnmapped.java
Show resolved
Hide resolved
| required_capability: optional_fields_nullify_tech_preview | ||
| required_capability: optional_fields_fix_unmapped_field_detection | ||
|
|
||
| SET unmapped_fields="nullify"; FROM date_nanos | SORT millis ASC | WHERE millis < "2000-01-01" | EVAL nanos = MV_MIN(nanos) | KEEP nanos; |
There was a problem hiding this comment.
nit: formatting, in csv tests we normally put every command into a separate line.
| required_capability: optional_fields_nullify_tech_preview | ||
| required_capability: optional_fields_fix_unmapped_field_detection |
There was a problem hiding this comment.
nit: if you require optional_fields_fix_unmapped_field_detection, you don't also need to require optional_fields_nullify_tech_preview.
| ; | ||
|
|
||
| keepMapped | ||
| required_capability: date_nanos_type |
There was a problem hiding this comment.
That capability is switched on for super long now (since #117080). That's old enough that any bwc test can assume that capability is permanently switched on.
Let's remove it - this test is skipped based on the optional_fields_fix_unmapped_field_detection capability, anyway.
| required_capability: optional_fields_nullify_tech_preview | ||
| required_capability: optional_fields_fix_unmapped_field_detection | ||
|
|
||
| SET unmapped_fields="nullify"; FROM date_nanos | SORT millis ASC | WHERE millis < "2000-01-01" | EVAL nanos = MV_MIN(nanos) | KEEP nanos; |
There was a problem hiding this comment.
nit: a small comment on the test would help, e.g. with a link to the issue that we close.
I presume this reproduces because ResolveRefs stops at the WHERE millis < "2000-01-01" and needs to pass control to ImplicitCasting before continuing - and in between, ResolveUnmapped is being run, wrongly assuming that any unresolved refs are due to missing fields that stop ResolveRefs from continuing.
alex-spies
left a comment
There was a problem hiding this comment.
Let's backport to 9.3, however. I'll add the labels, esp. the auto-backport one.
# Conflicts: # x-pack/plugin/esql/qa/server/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/multi_node/EsqlSpecIT.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
💔 Backport failed
You can use sqren/backport to manually backport by running |
(cherry picked from commit 1bb4312)
(cherry picked from commit 1bb4312)
This change fixes unexpected behavior when some columns are incorrectly nullified when using
SET unmapped_fields="nullify";Fixes: #141870