-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
SQL: Translate MIN/MAX on keyword fields as FIRST/LAST #41247
Conversation
Although the translation rule was implemented in the `Optimizer`, the rule was not added in the list of rules to be executed. Relates to elastic#41195 Follows elastic#37936
Pinging @elastic/es-search |
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.
Can you add a test to QueryTranslatorTests
specifically for MIN(string) and MAX(string), please?
And I would also like to see more tests in general, with these two for strings.
There are already tests for optimizer: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java#L1236 Would you like to add more integration tests? |
I missed those from OptimizerTests class; if you believe they are enough, it's ok with me. I was used to those in the QueryTranslatorTests class. Yes, more integration tests are welcomed. The more, the better. Thanks. |
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.
+1 on having a bit more integration tests - they don't hurt and will check potential mistakes.
Added also query translator tests and one more integ test. |
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
Although the translation rule was implemented in the `Optimizer`, the rule was not added in the list of rules to be executed. Relates to elastic#41195 Follows elastic#37936
Although the translation rule was implemented in the
Optimizer
and the docs are in place, the rule was not added in the list of rules
to be executed.
Relates to #41195
Follows #37936