ES|QL: Implicit numeric widening for union types#144288
ES|QL: Implicit numeric widening for union types#144288felixbarny wants to merge 3 commits intoelastic:mainfrom
Conversation
Add automatic numeric type widening for ES|QL union types, both in multi-index queries and UNION ALL (FORK) operations. When a field is mapped as different numeric types across indices (e.g. integer in one index, long in another), ES|QL now automatically widens to the common numeric type without requiring an explicit cast. Fixes elastic#138509
|
Hi @felixbarny, I've created a changelog YAML for you. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
alex-spies
left a comment
There was a problem hiding this comment.
Thanks @felixbarny !
I think the proposed change would be useful, but we chose not to perform auto-casting for union types for now and as I understand, the point of #138509 was to first form a consistent approach rather than immediately build and merge a fix. I think this PR will have implications on the overall semantics of ESQL which need to be understood well before we can merge this at the current scope it covers.
To be more explicit:
This PR addresses only some numerical types, but I expect that this sets a precedent that would be inconsistent not to follow-up on for other type combinations, which has a lot of implied scope further down the road. There are a lot of open questions on which types should be auto cast into each other once we start with the basic numerical types; for instance, should date->date_nanos be automatic? It already is in union types, but that was supposed to be an exception due to pragmatic concerns for data streams; and it was quite unclear, as date->nanos can lose dates outside the nanos range, so we thoroughly weighed risks when implementing this. But what about counter_long->long? What about ips and keywords? Keywords and versions? And, if the auto-casting is in place for union types, why should the rest of the language not follow suit?
Also, contrary to what the title suggests, this is not only about types that ESQL considers widened; short->integer is internally widened, whereas integer->long isn't. As in, we do instantiate blocks of distinct types for integers and longs. This is even more true for long->double, which can lose precision, but is automatically enabled in this PR by using EsqlDataTypeConverter::commonType.
There are also competing approaches being discussed; for instance, @quackaplop proposed a separate dynamice mode for ESQL that would turn how we deal with types upside down and would come with auto-casting as well. I'm sure this mode would also cover union types, so we'd have to ensure this change is consistent with the overall dynamic mode.
Related to this, in #142657 I plan to codify funciton polymorphism in ESQL. This would generally support this PR by first making the notion of "widening" a bit more precise, and taking a more opinionated stance on which type is considered "wider" than another - but this PR is still blocked on internal discussion.
That said, I do think that it is inconsistent that ESQL treats short as integer when using a single index, but when we have field mapped as short in one index and integer in another, ESQL gets fussy about it and considers this a union type that you need to manually cast.
If we reduce the scope of this PR to just stop considering integer/short/byte mapping conflicts as union types, but just treat that as a consistently mapped integer field, I can get behind the change as union types are being more restrictive than necessary. That is, rather than perform auto-casting of numerics to their commonType, I'd limit the scope to what DataType#widenSmallNumeric provides for now. Implementation-wise, this might actually get a lot simpler as well and may not need a new analyzer rule. Semantics-wise, it would mean that union types don't auto-resolve integer/long or long/double mapping conflicts automatically, which is consistent with the rest of the language, which doesn't auto-cast one into the other.
|
Thanks a lot for looking into this! This was meant more as a conversation starter. Let me convert it to draft to make it clearer. I've also added more context to the main issue around the use case and impact. Some thoughts inline
I think we can separate out the discussions about numeric types from other types. While it does set a precedent for numeric types, I things like keyword -> ip auto-conversion are completely orthogonal. The same way as Java does auto-widening of numerics but doesn't automatically convert But you're right that it implies more scope down the line. At least for counter types, for example.
Maybe I didn't use the right terminology here. From a user's perspective, I don't think it matters much whether we internally create a single numeric block or if we auto-convert for union types. I'd be happy with either approach if the outcome "feel" like auto-widening from a user's POV.
I think that would be too restrictive. We should handle |
Summary
NumericWidenInEsRelationanalyzer rule that implicitly resolvesInvalidMappedFieldinstances where all types are numeric, computing the common widened type viaEsqlDataTypeConverter.commonType()and injecting per-index conversion expressionsResolveUnionTypesInUnionAll.commonType()to handle numeric widening in UNION ALL / FORK operationsUNION_TYPES_IMPLICIT_NUMERIC_WIDENINGcapabilityWhen a field is mapped as different numeric types across indices (e.g.
integerin one index,longin another), ES|QL now automatically widens to the common numeric type without requiring an explicit cast likefield::long.Counter type limitation
Counter types (
counter_integer,counter_long,counter_double) are not handled by implicit numeric widening in this PR. Specifically:NumericWidenInEsRelationrule checksimf.types().stream().allMatch(DataType::isNumeric), and counter types returnfalseforisNumeric(), so fields with counter types across indices remainunsupported(same as before this PR).UNION ALL: TheResolveUnionTypesInUnionAll.commonType()method strips counter-ness before comparison (socounter_long+longstill resolves tolong, as before), but when two different counter types appear (e.g.,counter_integer+counter_long) or when a counter type is mixed with a non-counter of a different base type (e.g.,counter_integer+long), widening is explicitly skipped and the field remainsunsupported.The reason is that there are no counter-specific converter functions (
ToCounterLong,ToCounterInteger, etc.) in the converter framework, so widening cannot be expressed as a cast today. This will be addressed in a follow-up.Fixes #138509