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: Fix metric aggs on date/time to not return double #40377
Conversation
Previously metric aggregations on date fields would return a double which caused errors when trying to apply scalar functions on top, e.g.: ``` SELECT YEAR(MAX(date)) FROM test ``` Fixes: elastic#40376
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.
Good catch. LGTM!
Yet another instance of session information...
return tryExtractDateTime(innerKey != null && v instanceof Map ? ((Map<?, ?>) v).get(innerKey) : v); | ||
} | ||
|
||
private Object tryExtractDateTime(Object object) { |
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.
handleDateTime/maybeDateTime
sounds better.
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.
I know it's merged already, but left two comments.
this.name = name; | ||
this.property = property; | ||
this.innerKey = innerKey; | ||
this. isDateTimeBased =isDateTimeBased; |
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.
Weird spacing.
@@ -279,6 +279,8 @@ aggMinWithAlias | |||
SELECT gender g, MIN(emp_no) m FROM "test_emp" GROUP BY g ORDER BY gender; | |||
aggMinOnDateTime | |||
SELECT gender, MIN(birth_date) m FROM "test_emp" GROUP BY gender ORDER BY gender; |
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.
If this test here wasn't complaining before, this means that our SQL spec test case infrastructure isn't actually comparing an actual date type of data, right? Otherwise, the test would have failed.
If so, I would like to see a test with MIN(date_field)
in CSV spec test case infra, if possible. I have seen the unit test that compares the metric agg key with an actual date (assertEquals(DateUtils.asDateTime((long) innerValue , zoneId), extractor.extract(bucket));
) but I believe our tests would be more complete if there would be an integration test as well (CSV-spec in this case).
Previously metric aggregations on date fields would return a double which caused errors when trying to apply scalar functions on top, e.g.: ``` SELECT YEAR(MAX(date)) FROM test ``` Fixes: elastic#40376
Previously metric aggregations on date fields would return a double
which caused errors when trying to apply scalar functions on top, e.g.:
Fixes: #40376
Fixes: #39492