ESQL: fix non-null value being returned for unsupported data types in ValueSources #100656
Conversation
are returned from SourceValues. Improve unsupported data types handling in TopN
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
Hi @astefan, I've created a changelog YAML for you. |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
| sources.add( | ||
| new ValueSourceInfo( | ||
| new UnsupportedValueSourceType(fieldType.typeName()), | ||
| new UnsupportedValueSource(null), |
There was a problem hiding this comment.
This is the actual fix where instead of using null as original source argument, the actual source was still in use.
There was a problem hiding this comment.
Thanks for explicitly calling this out vs -> null. It would be easy to miss in review.
| @@ -380,6 +380,8 @@ private PhysicalOperation planTopN(TopNExec topNExec, LocalExecutionPlannerConte | |||
| case "version" -> TopNEncoder.VERSION; | |||
| case "boolean", "null", "byte", "short", "integer", "long", "double", "float", "half_float", "datetime", "date_period", | |||
| "time_duration", "object", "nested", "scaled_float", "unsigned_long", "_doc" -> TopNEncoder.DEFAULT_SORTABLE; | |||
| // unsupported fields are encoded as BytesRef, we'll use the same encoder; all values should be null at this point | |||
| case "unsupported" -> TopNEncoder.UNSUPPORTED; | |||
There was a problem hiding this comment.
This is a form of double-checking that non-null values are not reaching TopN for unsupported data types.
| @@ -99,19 +111,7 @@ public static List<ValueSourceInfo> sources( | |||
| var fieldContext = new FieldContext(fieldName, fieldData, fieldType); | |||
| var vsType = fieldData.getValuesSourceType(); | |||
| var vs = vsType.getField(fieldContext, null); | |||
|
|
|||
| if (asUnsupportedSource) { | |||
There was a problem hiding this comment.
This was moved earlier in the method because there is no need for all the checks before it, if we know that the data type of the field is definitely an unsupported one. As a consequence, more tests now have warnings added to the response header for cases where we return nulls for unsupported data types.
| sources.add( | ||
| new ValueSourceInfo( | ||
| new UnsupportedValueSourceType(fieldType.typeName()), | ||
| new UnsupportedValueSource(null), |
There was a problem hiding this comment.
Thanks for explicitly calling this out vs -> null. It would be easy to miss in review.
| @@ -380,6 +380,8 @@ private PhysicalOperation planTopN(TopNExec topNExec, LocalExecutionPlannerConte | |||
| case "version" -> TopNEncoder.VERSION; | |||
| case "boolean", "null", "byte", "short", "integer", "long", "double", "float", "half_float", "datetime", "date_period", | |||
| "time_duration", "object", "nested", "scaled_float", "unsigned_long", "_doc" -> TopNEncoder.DEFAULT_SORTABLE; | |||
| // unsupported fields are encoded as BytesRef, we'll use the same encoder; all values should be null at this point | |||
| case "unsupported" -> TopNEncoder.UNSUPPORTED; | |||
astefan
added
v8.10.0
auto-backport-and-merge
💔 Backport failed
You can use sqren/backport to manually backport by running |
… ValueSources (elastic#100656) * Refine (and fix) all cases where an unsupported data type field's values are returned from SourceValues. * Improve unsupported data types handling in TopN (cherry picked from commit 29e3d28)
Refine (and fix) all cases where an unsupported data type field's values are returned from ValueSources.
Improve unsupported data types handling in TopN by catching non-null values encoding/decoding attempts for unsupported data types.
Fixes #100048