-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
feat: support mulitple temporal filters in AdhocFilter and move the Time Section away #21767
feat: support mulitple temporal filters in AdhocFilter and move the Time Section away #21767
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21767 +/- ##
==========================================
+ Coverage 66.86% 66.97% +0.11%
==========================================
Files 1807 1811 +4
Lines 69218 69341 +123
Branches 7402 7442 +40
==========================================
+ Hits 46280 46441 +161
+ Misses 21033 20981 -52
- Partials 1905 1919 +14
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 |
dd6cd0c
to
acb0523
Compare
d05501d
to
8157fec
Compare
8ac69be
to
32f7b2e
Compare
9662a7c
to
448cd7e
Compare
a00e71d
to
bba9277
Compare
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.
First pass comments. Super stoked to finally see the time section being removed! Also, it would be really great if we could have some design eyes on the adhoc filter modal, as the time picker pill feels slightly sketchy in this particular context (even making the pill full width would be an improvement IMO)
if (hasGenericChartAxes && query.time_range) { | ||
// eslint-disable-next-line no-param-reassign | ||
query.filters = ensureIsArray(query.filters).map(flt => | ||
flt?.op === 'DATETIME_BETWEEN' | ||
? ({ ...flt, val: query.time_range } as BinaryQueryObjectFilterClause) | ||
: flt, | ||
); | ||
} |
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 assume this is for applying native/cross filters to the adhoc time filters? I think it would be really important to be able to specify to which adhoc time filters native/cross filters apply. In the adhoc filter modal we could have a checkbox under the time picker that says "Overridable by native time filter" or similar. Then if that field is unchecked, the query.time_range
would not apply to the filter.
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.
Yes, it's for applying native filters to the ad-hoc filter on the Dashboard, this part will override all ad-hoc time filters in the charts of the Dashboard.
In the original design is that the chart only has a single time filter so the Time Filter in the Native Filter was designed only to pass time_range
to every chart rather than pass time column
(granularity in the QueryObject) and time value
(time_range in the QueryObject) to the chart.
To solve this issue, I think we might need to specify the time columns in the Native Filter so that the user knows which time column would apply.
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 think there's two distinct use cases here:
- Emitting a generic temporal value to be applied to a chart's own temporal column - this is the behavior of the current time filters (both dashboard native filters and filter box). This is handy when charts may have different temporal columns, for instance
created_at
orupdated_at
, and we don't want to change the temporal column, but rather just replace the value of the temporal filter. - emitting a time filter for a specific temporal column. This would make it possible to have, for instance, two different time filters with different underlying columns, e.g.
created_at
andupdated_at
, and emit those filters along with the target column to all charts that are in scope.
I believe there's probably a need for both, and we may need to revisit the second option at some point, but for now let's at least make sure we don't break support for the first case.
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts
Show resolved
Hide resolved
if tt in (utils.TemporalType.TEXT, utils.TemporalType.DATETIME): | ||
if tt in ( | ||
utils.TemporalType.TEXT, | ||
utils.TemporalType.DATETIME, | ||
utils.TemporalType.TIMESTAMP, | ||
): |
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.
by catch: SQLite also has a timestamp type.
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.
Not doing a full review because most I'm pretty out of my depth on most of the code changes, but I tested locally on several ECharts and it seems to work great! I did see a few issues, but they were also present on master so not sure if they're relevant here:
- "Original Value" option in time grain is a little buggy for me, if you select it the selection disappears but if you select it again it appears.
- For charts with "Temporal X-axis", you can't remove the default item with the little "x". If there are two options and you choose one that's not default, then click the "x", it'll just go back to the original one.
- Some charts had a tooltip value that seemed off:
My one other comment is that checking feature flags at module execution time has been responsible for some weird bugs in the past due to race conditions between when they're set and read (that's what #21315 was fixing). It would be great if we could only check feature flags during React rendering or other execution points that are guaranteed to happen after initial module execution. That honestly looks pretty hard to do in this PR, but just wanted to note that this might be a source of bugs in the future.
323e08d
to
9cc39c9
Compare
@zhaoyongjie Ephemeral environment spinning up at http://54.188.24.204:8080. Credentials are |
Thank you for the great work~ Expect: Actual: Screen.Recording.2022-11-01.at.8.36.28.AM.mov |
Hi @jinghua-qa, the |
Are we going to support this feature in the future? it looks like when we move the time range to filter, user can use custom sql to support time column as filter, it will be great if we can support that with time comparison. |
If we want to support This part of the work is completely beyond the scope of this PR. |
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
Ephemeral environment shutdown and build artifacts deleted. |
Two questions on this feature:
|
Any update on this ?? |
What version of Superset do you use? Native filter in the Dashboard should append a new time filter, so I think it should be worked on Dashboard.
Should support Jinja Template if not, it should be a regression bug. |
ideally it should give result in the form of timestamp so it can be easily used in sql just like from_dttm and to_dttm. Can we parse above values to get timestamp from above format effectively ?
|
The val in each time filter means that dynamically calculate "from date time" and "to date time". If you just want to use fixed timestamp, leave timestamp string in Custom, for instance:
It might be a regression bug, I've revert this PR in my custom Superset. I hope it should help with you. |
@bilalabdullah44000 posted an incorrect link, just updated |
I Understand. if i select timecolumn , then the time range filter will be applied on the column selected in the time column. but how do i apply multiple timerange filters i.e. filter on both created_by column and updated_by column at dashboard level ? |
@zhaoyongjie Can we apply multiple time range filters at the same time on dashboard as well like slice explore ? |
IMO, Dashboard must be applied multiple time range filter. If not, it's a bug. |
Here in attached screenshot, I have applied two time range filters i.e. created_at and updated_at. As seen in the query, Only one filter is applied on default temporal column or on the column which is selected in time column field in dashboard filters section. |
@bilalabdullah44000 You need to use Time Column filter and Time Range filter together to select multiple time filter, but I also found this feature was not correct work on the master(git hash:4881328fb), the Time Range replaced all of the value of time filters. I suggest that the first step revert the PR #23021 and then try to fix it in your own side. Multiple time filters in the Explore page, one is Last Month, another is Last YearMultiple time filters on Dashboard was not work |
By this feature, I think we should redesign the Time Filter on the Dashboard, should merge the Time Column/Time Range/Time Grain together as a unique Time Filter. Unfortunately, the new Time Filter almost didn't support multiple filters on the Dashboard, and some codebase was broken previous design, ---- the Native Filter should APPEND the new where clause rather than REPLACE value of filter of chart. |
Background
The Time Section is in the Superset Explore now. The original design is for selecting Druid time partition and time grains.
Over time, Superset has supported more databases. The time filter should be a kind of Adhoc Filters.
Summary
This PR will move functionality of the Time Section to the Adhoc Filters when
GENERIC_CHART_AXES
is enabled, and then Adhoc Filters should support multiple time columns.The new operator in the Adhoc filter that is
TEMPORAL_BETWEEN
has been added, so the newqueryObject
will support like thisthe filter will generate a SQL snippet
Unsupported Charts
multiple.temporal.columns.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION