-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a form of double-checking that non-null values are not reaching TopN for unsupported data types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 null
s for unsupported data types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @astefan!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sources.add( | ||
new ValueSourceInfo( | ||
new UnsupportedValueSourceType(fieldType.typeName()), | ||
new UnsupportedValueSource(null), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
💔 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