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

[ESQL] SimplifyComparisonArithmetics can cause nulls on valid queries #108519

Open
not-napoleon opened this issue May 10, 2024 · 1 comment
Open
Assignees
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@not-napoleon
Copy link
Member

Elasticsearch Version

main

Installed Plugins

No response

Java Version

bundled

OS Version

n/a

Problem Description

The SimplifyComparisonArithmetics optimization incorrectly applies to queries like FROM test | WHERE int - 1 < 9223372036854775807" (That's Long.MAX_VALUE, to save your eyes). The optimization's tryFolding method expects that if the folding has an error, it will throw an appropriate exception, which the optimization then catches and simply returns the unoptimized input expression. However, ESQL doesn't throw from folding, so this check never applies. Instead, we get back a Literal with a null value. We can't just check for a null value, because the warning about the error will already be on the query at that point, even if we then don't apply the optimization, so it's not a trivial fix.

Steps to Reproduce

PUT http://localhost:9200/test
content-type: application/json

{
  "mappings": {
    "properties": {
        "float": {"type": "float"},
        "int": {"type": "integer"},
        "double": {"type": "double"}
    }
  }
}


PUT http://localhost:9200/test/_doc/1
content-type: application/json

{
    "int": "11",
    "float": "3.14",
    "double": "42.42"
}

POST http://localhost:9200/_query?format=txt
content-type: application/json

{
  "version": "2024.04.01",
  "query": "FROM test | WHERE int < 9223372036854775807 + 1" 
}

Logs (if relevant)

No response

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 10, 2024
@elasticsearchmachine
Copy link
Collaborator

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

not-napoleon added a commit that referenced this issue Jun 4, 2024
This lays the groundwork for fixing #108519, although I don't intend to fix it in this PR. In order to address that issue, we need to be able to control at call time how we deal with warnings, but, prior to this work, the ExpressionEvaluators all hard code their warnings implementation.

This PR will allow injecting in a different implementation when we create the evaluator. Unfortunately, this has fallen slightly short of that ambition. We need to be able to construct evaluators from within compute, but the Warnings class, and it's Source dependency, are in the esql package, which we don't want to import into compute. As result, I've pared this back to just passing in a toggle for warnings collection, via a no-op collector implementation. This is sufficient to solve the linked issue (still not done in this PR) and preserves our current encapsulation, while leaving a clear path to further refine how we control warnings in the future.

Note that this is changing the code generation, which means it will touch all of the generated files, which are still in source control. The actual change is much smaller than the diff would imply.

Replaces #108635
dnhatn pushed a commit to dnhatn/elasticsearch that referenced this issue Jun 4, 2024
This lays the groundwork for fixing elastic#108519, although I don't intend to fix it in this PR. In order to address that issue, we need to be able to control at call time how we deal with warnings, but, prior to this work, the ExpressionEvaluators all hard code their warnings implementation.

This PR will allow injecting in a different implementation when we create the evaluator. Unfortunately, this has fallen slightly short of that ambition. We need to be able to construct evaluators from within compute, but the Warnings class, and it's Source dependency, are in the esql package, which we don't want to import into compute. As result, I've pared this back to just passing in a toggle for warnings collection, via a no-op collector implementation. This is sufficient to solve the linked issue (still not done in this PR) and preserves our current encapsulation, while leaving a clear path to further refine how we control warnings in the future.

Note that this is changing the code generation, which means it will touch all of the generated files, which are still in source control. The actual change is much smaller than the diff would imply.

Replaces elastic#108635
@not-napoleon not-napoleon self-assigned this Jun 5, 2024
craigtaverner pushed a commit to craigtaverner/elasticsearch that referenced this issue Jun 11, 2024
This lays the groundwork for fixing elastic#108519, although I don't intend to fix it in this PR. In order to address that issue, we need to be able to control at call time how we deal with warnings, but, prior to this work, the ExpressionEvaluators all hard code their warnings implementation.

This PR will allow injecting in a different implementation when we create the evaluator. Unfortunately, this has fallen slightly short of that ambition. We need to be able to construct evaluators from within compute, but the Warnings class, and it's Source dependency, are in the esql package, which we don't want to import into compute. As result, I've pared this back to just passing in a toggle for warnings collection, via a no-op collector implementation. This is sufficient to solve the linked issue (still not done in this PR) and preserves our current encapsulation, while leaving a clear path to further refine how we control warnings in the future.

Note that this is changing the code generation, which means it will touch all of the generated files, which are still in source control. The actual change is much smaller than the diff would imply.

Replaces elastic#108635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

No branches or pull requests

2 participants