Skip to content
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

[ES|QL] Implicit numeric casting for CASE/GREATEST/LEAST #122601

Conversation

fang-xing-esql
Copy link
Member

Resolves #121890

#111917 supports implicit casting for numeric values and fields in COALEASE, the subsequent numeric arguments are casted to the first not null numeric type argument. This PR follow the same path to support CASE, GREATEST and LEAST to have the same behavior as COALESCE.

For example,

  1. GREATEST(Long, Int) returns a Long

  2. LEAST(null, Long, Int) returns a Long

  3. CASE(condition1, Double, condition2, Long, Int) returns a Double

  4. GREATEST(Int, Long) returns VerificationException, because Long cannot be casted to Int.

Technically, we can make the implicit casting independent of the sequence how the numeric values and fields are coded in the query, so that the query #4 above can run to completion, if this is the desired behavior.

@fang-xing-esql fang-xing-esql added auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.19.0 v9.1.0 labels Feb 14, 2025
@fang-xing-esql fang-xing-esql marked this pull request as ready for review February 18, 2025 20:35
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 18, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Hi @fang-xing-esql, I've created a changelog YAML for you.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM however we should look into generalizing this by looking at the param declaration instead of specific functions.

@fang-xing-esql
Copy link
Member Author

LGTM however we should look into generalizing this by looking at the param declaration instead of specific functions.

Thanks for reviewing @costin , #123088 is created for using annotation to tag function parameters that are eligible for implicit casting.

@fang-xing-esql fang-xing-esql merged commit 412e6c2 into elastic:main Feb 21, 2025
17 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.0 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122601

@fang-xing-esql
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.x
9.0

Questions ?

Please refer to the Backport tool documentation

fang-xing-esql added a commit to fang-xing-esql/Elasticsearch that referenced this pull request Feb 21, 2025
)

* implicit numeric casting for conditional functions

(cherry picked from commit 412e6c2)

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
elasticsearchmachine pushed a commit that referenced this pull request Feb 21, 2025
…123089)

* implicit numeric casting for conditional functions

(cherry picked from commit 412e6c2)

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
elasticsearchmachine pushed a commit that referenced this pull request Feb 21, 2025
…123091)

* implicit numeric casting for conditional functions

(cherry picked from commit 412e6c2)

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
@alex-spies
Copy link
Contributor

@fang-xing-esql just to double check, since we're backporting this to 9.0, did we intend to also backport this to 8.18 (which is kinda-sorta in sync with 9.0)? Not that this is a technical requirement, just something that stood out to me.

@fang-xing-esql
Copy link
Member Author

@fang-xing-esql just to double check, since we're backporting this to 9.0, did we intend to also backport this to 8.18 (which is kinda-sorta in sync with 9.0)? Not that this is a technical requirement, just something that stood out to me.

Good point! I'll back port this to 8.18 as well.

@fang-xing-esql
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.18

Questions ?

Please refer to the Backport tool documentation

elasticsearchmachine pushed a commit that referenced this pull request Feb 24, 2025
…123305)

* implicit numeric casting for conditional functions

(cherry picked from commit 412e6c2)

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.0.0 v9.0.1 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES|QL implicit typecasting missing in CASE
4 participants