feat: implement time grain in temporal filters #24035
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
villebro
commented
May 12, 2023
| @@ -1945,7 +1958,6 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma | |||
| inner_groupby_exprs = [] | |||
| inner_select_exprs = [] | |||
| for gby_name, gby_obj in groupby_series_columns.items(): | |||
| label = get_column_name(gby_name) | |||
There was a problem hiding this comment.
bycatch - this variable was not used anywhere (apparently this function is so bloated nowadays that the linters just give up on it)
Codecov Report
@@ Coverage Diff @@
## master #24035 +/- ##
=======================================
Coverage 68.21% 68.22%
=======================================
Files 1941 1941
Lines 75259 75261 +2
Branches 8168 8168
=======================================
+ Hits 51341 51344 +3
+ Misses 21829 21828 -1
Partials 2089 2089
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
villebro
commented
May 12, 2023
| @@ -1353,6 +1363,7 @@ def get_timestamp_expression( | |||
| """ | |||
| Return a SQLAlchemy Core element representation of self to be used in a query. | |||
|
|
|||
| :param column: column object | |||
There was a problem hiding this comment.
bycatch - missing docstring
villebro
commented
May 12, 2023
| "PT1S": "CAST({col} as TIMESTAMP)" " - MICROSECOND({col}) MICROSECONDS", | ||
| "PT1S": "CAST({col} as TIMESTAMP) - MICROSECOND({col}) MICROSECONDS", |
There was a problem hiding this comment.
cleanup - that split is no longer needed:
>>> "CAST({col} as TIMESTAMP)" " - MICROSECOND({col}) MICROSECONDS" == "CAST({col} as TIMESTAMP) - MICROSECOND({col}) MICROSECONDS"
True
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
While the
QueryObjectFilterClausecurrently supports passing agrain(see here:superset/superset/utils/core.py
Lines 223 to 228 in 7fe0ca1
2000-01-01(Saturday) becomes1999-12-26(the previous Monday) with weekly timegrain. Without applying the time grain to the temporal range, the observation for the first week in the query will only consist of two days (Saturday and Sunday), which can give the impression that the events for Monday-Friday never took place.While writing the test I noticed we had many redundant split strings (they were likely split before due to line width, but were moved on the same line by black). So I cleaned up the ones I found under
db_engine_specs.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION