You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/10/20 06:40:06 UTC
[GitHub] [superset] zhaoyongjie opened a new pull request, #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
zhaoyongjie opened a new pull request, #21767:
URL: https://github.com/apache/superset/pull/21767
#### Background
The Time Section is present in the Superset Explore. 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 `DATETIME_BETWEEN` has been added, so the new `queryObject` will support like this
```
{
...
filters: {
"col": "Order Date",
"op": "DATETIME_BETWEEN",
"val": "DATEADD(DATETIME(\"2022-10-10T00:00:00\"), -20, year) : 2022-10-10T00:00:00"
}
...
}
```
the filter will generate a SQL snippet
```SQL
...
WHERE "Order Date" >= '2002-10-10 00:00:00.000000'
AND "Order Date" < '2022-10-10 00:00:00.000000'
...
```
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1296128845
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1287016796
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1632377928
> @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, 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 side.
>
> <img alt="image" width="1398" src="https://user-images.githubusercontent.com/2016594/252954541-d311b2ca-fcc6-46e4-9802-d79189435d70.png">
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1283875494
> "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.
Yes, It's a bug because the `Original value` and unselected shared the same key(`null`) in the selection control. The easiest way to fix this is disabled removing the value in the time grain selection. I will fix that in the separate PR because it's also on the master branch.
> 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.
What I'm trying to say is, this is all original Druid logic. There is no default time column in the general-purpose database or general BI. It is a little weird design to always place a time column for a generic axis control (which is not a temporal x-axis). this is only my personal view, welcome to discuss.
> Some charts had a tooltip value that seemed off
Good catch, I will check it out.
> checking feature flags
Feature flags are global static constant for the frontend codebase, these values can't be reset by the frontend codes because they're Flask application context config items, in the other words, the only way to change these configs that need to restart the Superset process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie closed pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie closed pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
URL: https://github.com/apache/superset/pull/21767
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #21767: feat: support mulitple datetime filter in AdhocFilter and move away time section
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1274554626
# [Codecov](https://codecov.io/gh/apache/superset/pull/21767?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21767](https://codecov.io/gh/apache/superset/pull/21767?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f6a171f) into [master](https://codecov.io/gh/apache/superset/commit/db075d4157d57e28b5feee987a544c0e65b406a1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db075d4) will **increase** coverage by `10.22%`.
> The diff coverage is `68.83%`.
> :exclamation: Current head f6a171f differs from pull request most recent head dd6cd0c. Consider uploading reports for the commit dd6cd0c to get more accurate results
```diff
@@ Coverage Diff @@
## master #21767 +/- ##
===========================================
+ Coverage 55.30% 65.52% +10.22%
===========================================
Files 1800 1800
Lines 68967 68974 +7
Branches 7339 7339
===========================================
+ Hits 38144 45198 +7054
+ Misses 28931 21884 -7047
Partials 1892 1892
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.93% <31.57%> (?)` | |
| mysql | `78.24% <31.57%> (?)` | |
| presto | `52.83% <31.57%> (+<0.01%)` | :arrow_up: |
| python | `78.71% <31.57%> (+21.13%)` | :arrow_up: |
| sqlite | `76.79% <31.57%> (?)` | |
| unit | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21767?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...d/packages/superset-ui-chart-controls/src/index.ts](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2luZGV4LnRz) | `100.00% <ø> (ø)` | |
| [...chart-controls/src/shared-controls/dndControls.tsx](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `58.33% <0.00%> (ø)` | |
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `54.23% <ø> (ø)` | |
| [...t-ui-chart-controls/src/shared-controls/mixins.tsx](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9taXhpbnMudHN4) | `33.33% <0.00%> (ø)` | |
| [...kages/superset-ui-core/src/query/types/Operator.ts](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvT3BlcmF0b3IudHM=) | `100.00% <ø> (ø)` | |
| [.../BigNumber/BigNumberWithTrendline/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvY29udHJvbFBhbmVsLnRzeA==) | `16.66% <ø> (ø)` | |
| [...chart-echarts/src/MixedTimeseries/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL2NvbnRyb2xQYW5lbC50c3g=) | `54.16% <ø> (ø)` | |
| [.../plugin-chart-echarts/src/MixedTimeseries/index.ts](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL2luZGV4LnRz) | `25.00% <ø> (ø)` | |
| [.../plugin-chart-echarts/src/Timeseries/Area/index.ts](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9BcmVhL2luZGV4LnRz) | `33.33% <ø> (ø)` | |
| [...-chart-echarts/src/Timeseries/Regular/Bar/index.ts](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9SZWd1bGFyL0Jhci9pbmRleC50cw==) | `33.33% <ø> (ø)` | |
| ... and [384 more](https://codecov.io/gh/apache/superset/pull/21767/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996982674
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
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 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996982674
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1625299458
> Two questions on this feature:
>
> 1. How does this work in dashboards. Even if I add two time range filters in a dashboard, it is not possible to select the correct date columns
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.
> 2. How does this feature work with jinja templates? Are there any other variables than `{{ from_dttm }}` and `{{ to_dttm }}` to support this feature?
Should support Jinja Template if not, it should be a regression bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple datetime filter in AdhocFilter and move away time section
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996934120
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
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.
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -45,6 +45,7 @@ export enum Operators {
LATEST_PARTITION = 'LATEST_PARTITION',
IS_TRUE = 'IS_TRUE',
IS_FALSE = 'IS_FALSE',
+ DATETIME_BETWEEN = 'DATETIME_BETWEEN',
Review Comment:
Could we reconsider this name, as isn't "between" just one of the possible time filter types? Also, a temporal column may not necessarily be `DATETIME`, but could equally well be `DATE` or `TIMESTAMP`. So something like `TEMPORAL` would IMO be better.
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -18,6 +18,7 @@
*/
import { buildQueryContext } from '@superset-ui/core';
import * as queryModule from '../../src/query/normalizeTimeColumn';
+import * as getXAxisModule from '../../src/query/getXAxis';
Review Comment:
I know this is following the same convention as on the line above, but I personally prefer to avoid `import *` whenever possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1298793987
> Thank you for the great work~ I have a question, it looks like when i use custom sql in filter with time column, time comparison did not support, is this expected? Steps: 1, add a time column to filter 2, use time comparison to define start and end 3, go to time comparison and select compare time
Hi @jinghua-qa, the `Time Comparision` + `Custom SQL` doesn't been supported yet. Currently, the `Time Comparision` should with `time range` in the Adhoc filter rather than `Custom SQL`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000578862
##########
superset/common/query_object.py:
##########
@@ -336,6 +335,7 @@ def to_dict(self) -> Dict[str, Any]:
"series_limit": self.series_limit,
"series_limit_metric": self.series_limit_metric,
"to_dttm": self.to_dttm,
+ "time_shift": self.time_shift,
Review Comment:
`time_shift` is not really useful in existing code. It seems to be a parameter used by the old-time picker, and I'm only using it here to ensure backward compatibility for the query object.
The new time picker has supported `dateadd(datetime)` expression to achieve this function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1285321888
@zhaoyongjie Ephemeral environment spinning up at http://54.245.33.20:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bilalabdullah44000 commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "bilalabdullah44000 (via GitHub)" <gi...@apache.org>.
bilalabdullah44000 commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1626994811
@zhaoyongjie Can we apply multiple time range filters at the same time on dashboard as well like slice explore ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1625336863
> > > Two questions on this feature:
> > >
> > > 1. How does this work in dashboards. Even if I add two time range filters in a dashboard, it is not possible to select the correct date columns
> >
> >
> > 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.
> > > 2. How does this feature work with jinja templates? Are there any other variables than `{{ from_dttm }}` and `{{ to_dttm }}` to support this feature?
> >
> >
> > Should support Jinja Template if not, it should be a regression bug.
>
> 1. In jinja, it is giving result in the form of
> {
> ...
> filters: {
> "col": "Date1",
> "op": "TEMPORAL_RANGE",
> "val": "DATEADD(DATETIME("2022-10-10T00:00:00"), -20, year) : 2022-10-10T00:00:00"
> },
> {
> "col": "Date1",
> "op": "TEMPORAL_RANGE",
> "val": "Last day : 2022-10-10T00:00:00"
> }
> ```
> }
> ```
>
> 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:
<img width="991" alt="image" src="https://github.com/apache/superset/assets/2016594/367a638c-a08a-4acd-b617-8f2fe03c21b6">
>
> 2. Also at my end, if i create two dashboard filters i.e. created_at and updated_at , it is applying filter values against these 2 filters on my default datetime/temporal column i.e. last_state_change_time
>
> 
It might be a regression bug, I've revert this [PR](https://github.com/apache/superset/pull/24176) in my custom Superset. I hope it should help with you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996982674
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996991360
##########
superset/db_engine_specs/sqlite.py:
##########
@@ -77,7 +77,11 @@ def convert_dttm(
cls, target_type: str, dttm: datetime, db_extra: Optional[Dict[str, Any]] = None
) -> Optional[str]:
tt = target_type.upper()
- if tt in (utils.TemporalType.TEXT, utils.TemporalType.DATETIME):
+ if tt in (
+ utils.TemporalType.TEXT,
+ utils.TemporalType.DATETIME,
+ utils.TemporalType.TIMESTAMP,
+ ):
Review Comment:
by catch: SQLite also has a timestamp type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1287042548
@zhaoyongjie Ephemeral environment spinning up at http://35.87.79.248:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1006660668
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getTemporalColumns.ts:
##########
@@ -29,9 +29,9 @@ import {
isQueryResponse,
} from '@superset-ui/chart-controls';
-export const getTemporalColumns = (
+export function getTemporalColumns(
Review Comment:
slightly unrelated - it would be nice to have an ESLint rule for this to make the JS/TS code more consistent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1297158905
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1298756647
Thank you for the great work~
I have a question, it looks like when i use custom sql in filter with time column, time comparison did not support, is this expected?
Steps:
1, add a time column to filter
2, use time comparison to define start and end
3, go to time comparison and select compare time
Expect:
time comparison will work with time filter
Actual:
time comparison did not work with time filter
https://user-images.githubusercontent.com/81597121/199279123-f07f150d-7db2-4149-ae2d-1e87f9253451.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000588728
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -45,6 +45,7 @@ export enum Operators {
LATEST_PARTITION = 'LATEST_PARTITION',
IS_TRUE = 'IS_TRUE',
IS_FALSE = 'IS_FALSE',
+ DATETIME_BETWEEN = 'DATETIME_BETWEEN',
Review Comment:
the `TEMPORAL` sounds like a date type rather than an operator. how about `TEMPORAL_FROM` or `TEMPORAL_PARSER` or `CACULATE_TEMPORAL`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000289460
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -18,6 +18,7 @@
*/
import { buildQueryContext } from '@superset-ui/core';
import * as queryModule from '../../src/query/normalizeTimeColumn';
+import * as getXAxisModule from '../../src/query/getXAxis';
Review Comment:
Oh yes, makes sense 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1294504420
HI @villebro, I have addressed the comments that should populate the default time column into Adhoc Filters and the `DEFAULT_TIME_FILTER` could be set from `config.py`. I'd really appreciate your eyes on the last review codes when you have time.
IMO, the default column in the Dataset and the default filter value might be redesigned so that it supports the non-temporal column as the default filter.
https://user-images.githubusercontent.com/2016594/198513874-e1bc505d-e5c9-4f8f-a68e-23773b1bc8b0.mov
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged PR #21767:
URL: https://github.com/apache/superset/pull/21767
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1625340977
@bilalabdullah44000 posted an incorrect link, just updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000578862
##########
superset/common/query_object.py:
##########
@@ -336,6 +335,7 @@ def to_dict(self) -> Dict[str, Any]:
"series_limit": self.series_limit,
"series_limit_metric": self.series_limit_metric,
"to_dttm": self.to_dttm,
+ "time_shift": self.time_shift,
Review Comment:
`time_shift` is not really useful in existing code. It seems to be a parameter used by the old time picker, and I'm only using it here to ensure backward compatibility for the query object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1299386615
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000667939
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -45,6 +45,7 @@ export enum Operators {
LATEST_PARTITION = 'LATEST_PARTITION',
IS_TRUE = 'IS_TRUE',
IS_FALSE = 'IS_FALSE',
+ DATETIME_BETWEEN = 'DATETIME_BETWEEN',
Review Comment:
How do you feel about `TEMPORAL_RANGE`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1285118811
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996982674
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bilalabdullah44000 commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "bilalabdullah44000 (via GitHub)" <gi...@apache.org>.
bilalabdullah44000 commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1625376818
> > > > Two questions on this feature:
> > > >
> > > > 1. How does this work in dashboards. Even if I add two time range filters in a dashboard, it is not possible to select the correct date columns
> > >
> > >
> > > 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.
> > > > 2. How does this feature work with jinja templates? Are there any other variables than `{{ from_dttm }}` and `{{ to_dttm }}` to support this feature?
> > >
> > >
> > > Should support Jinja Template if not, it should be a regression bug.
> >
> >
> >
> > 1. In jinja, it is giving result in the form of
> > {
> > ...
> > filters: {
> > "col": "Date1",
> > "op": "TEMPORAL_RANGE",
> > "val": "DATEADD(DATETIME("2022-10-10T00:00:00"), -20, year) : 2022-10-10T00:00:00"
> > },
> > {
> > "col": "Date1",
> > "op": "TEMPORAL_RANGE",
> > "val": "Last day : 2022-10-10T00:00:00"
> > }
> > ```
> > }
> > ```
> >
> > 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:
>
> <img alt="image" width="991" src="https://user-images.githubusercontent.com/2016594/251737774-367a638c-a08a-4acd-b617-8f2fe03c21b6.png">
> > 2. Also at my end, if i create two dashboard filters i.e. created_at and updated_at , it is applying filter values against these 2 filters on my default datetime/temporal column i.e. last_state_change_time
> >
> > 
>
> It might be a regression bug, I've revert this [PR](https://github.com/apache/superset/pull/23021) in my custom Superset. I hope it should help with you.
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000575504
##########
superset/models/helpers.py:
##########
@@ -1348,6 +1348,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma
row_offset: Optional[int] = None,
timeseries_limit: Optional[int] = None,
timeseries_limit_metric: Optional[Metric] = None,
+ time_shift: Optional[str] = None,
Review Comment:
IMO, This helper has to do full refactoring. otherwise, all our code would have to be duplicated. the `time_shift` here is just to prevent raising an error from function calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1297166435
@zhaoyongjie Ephemeral environment spinning up at http://54.188.24.204:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1290902660
@zhaoyongjie Ephemeral environment spinning up at http://34.220.70.115:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] korpa commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "korpa (via GitHub)" <gi...@apache.org>.
korpa commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1543589287
Two questions on this feature:
1) How does this work in dashboards. Even if I add two time range filters in a dashboard, it is not possible to select the correct date columns
2) How does this feature work with jinja templates? Are there any other variables than `{{ from_dttm }}` and `{{ to_dttm }}` to support this feature?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bilalabdullah44000 commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "bilalabdullah44000 (via GitHub)" <gi...@apache.org>.
bilalabdullah44000 commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1625285957
> Two questions on this feature:
>
> 1. How does this work in dashboards. Even if I add two time range filters in a dashboard, it is not possible to select the correct date columns
> 2. How does this feature work with jinja templates? Are there any other variables than `{{ from_dttm }}` and `{{ to_dttm }}` to support this feature?
Any update on this ??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bilalabdullah44000 commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "bilalabdullah44000 (via GitHub)" <gi...@apache.org>.
bilalabdullah44000 commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1625322195
> > Two questions on this feature:
> >
> > 1. How does this work in dashboards. Even if I add two time range filters in a dashboard, it is not possible to select the correct date columns
>
> 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.
>
> > 2. How does this feature work with jinja templates? Are there any other variables than `{{ from_dttm }}` and `{{ to_dttm }}` to support this feature?
>
> Should support Jinja Template if not, it should be a regression bug.
1) In jinja, it is giving result in the form of
{
...
filters: {
"col": "Date1",
"op": "TEMPORAL_RANGE",
"val": "DATEADD(DATETIME(\"2022-10-10T00:00:00\"), -20, year) : 2022-10-10T00:00:00"
},
{
"col": "Date1",
"op": "TEMPORAL_RANGE",
"val": "Last day : 2022-10-10T00:00:00"
}
}
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 ?
2) Also at my end, if i create two dashboard filters i.e. created_at and updated_at , it is applying filter values against these 2 filters on my default datetime/temporal column i.e. last_state_change_time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1632371321
@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, the Time Range replaced all of the value of time filters. I suggest that the first step revert the PR https://github.com/apache/superset/pull/23021 and then try to fix it in your side.
<img width="1398" alt="image" src="https://github.com/apache/superset/assets/2016594/d311b2ca-fcc6-46e4-9802-d79189435d70">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000297686
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
I think there's two distinct use cases here:
1. 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` or `updated_at`, and we don't want to change the temporal column, but rather just replace the value of the temporal filter.
2. 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` and `updated_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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000328351
##########
superset/common/query_object.py:
##########
@@ -336,6 +335,7 @@ def to_dict(self) -> Dict[str, Any]:
"series_limit": self.series_limit,
"series_limit_metric": self.series_limit_metric,
"to_dttm": self.to_dttm,
+ "time_shift": self.time_shift,
Review Comment:
I believe this will invalidate all currently stored cache values: to avoid cache misses, I've previously placed a check on `QueryObject.cache_key` checking if the new property is truthy, and if so, add a new key to the cache key dict (the idea is to move all of these in one go to `QueryObject.to_dict()` to make a big bang during e.g. a major release).
##########
superset/common/query_context_processor.py:
##########
@@ -314,30 +316,49 @@ def processing_time_offsets( # pylint: disable=too-many-locals,too-many-stateme
rv_dfs: List[pd.DataFrame] = [df]
time_offsets = query_object.time_offsets
- outer_from_dttm = query_object.from_dttm
- outer_to_dttm = query_object.to_dttm
+ outer_from_dttm, outer_to_dttm = get_since_until_from_query_object(query_object)
+ if not outer_from_dttm or not outer_to_dttm:
+ raise QueryObjectValidationError(
+ _(
+ "An enclosed time range (both start and end) must be specified "
+ "when using a Time Comparison."
+ )
+ )
Review Comment:
Out of scope for this PR, but I believe we could remove this requirement, as I feel we should be able to do time comparison without specifying a time range (I usually work around this by placing a `100 years ago : now` on the time filter)
##########
superset/models/helpers.py:
##########
@@ -1348,6 +1348,7 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma
row_offset: Optional[int] = None,
timeseries_limit: Optional[int] = None,
timeseries_limit_metric: Optional[Metric] = None,
+ time_shift: Optional[str] = None,
Review Comment:
I'm not familiar with these helpers, but do we need to also copy over the new logic from `get_sqla_query()` in `connectors/sqla/models.py`? Or better yet, remove one of these to avoid code duplication?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000677043
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -45,6 +45,7 @@ export enum Operators {
LATEST_PARTITION = 'LATEST_PARTITION',
IS_TRUE = 'IS_TRUE',
IS_FALSE = 'IS_FALSE',
+ DATETIME_BETWEEN = 'DATETIME_BETWEEN',
Review Comment:
Like this. Thanks to InteliJ, renaming is done in 1 second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1294564498
> LGTM. Comparing the old UX with the new one, I think this new behavior is much more intuitive than the last one + this brings the much anticipated functionality of multiple time filters with a very intuitive UX flow. Thanks for the incredible effort on this one, this is a major milestone in finally shedding some of the last and most annoying remnants of the legacy Druid connector! ❤️ 🚀
Thanks so much for the review. You make my day.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1290595800
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1298798777
> > Thank you for the great work~ I have a question, it looks like when i use custom sql in filter with time column, time comparison did not support, is this expected? Steps: 1, add a time column to filter 2, use time comparison to define start and end 3, go to time comparison and select compare time
>
> Hi @jinghua-qa, the `Time Comparision` + `Custom SQL` doesn't been supported yet. Currently, the `Time Comparision` should with `time range` in the Adhoc filter rather than `Custom SQL`.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1296129759
@jinghua-qa Ephemeral environment spinning up at http://34.218.209.219:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1298804072
> > > Thank you for the great work~ I have a question, it looks like when i use custom sql in filter with time column, time comparison did not support, is this expected? Steps: 1, add a time column to filter 2, use time comparison to define start and end 3, go to time comparison and select compare time
> >
> >
> > Hi @jinghua-qa, the `Time Comparision` + `Custom SQL` doesn't been supported yet. Currently, the `Time Comparision` should with `time range` in the Adhoc filter rather than `Custom SQL`.
>
> 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 `custom SQL` + `Time Comparison`, there are some pre-requirement works, e.g. 1) parsing and validating it. 2) it should contain the start time and end time. This part of the work is completely beyond the scope of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1294566657
> IMO, the default column in the Dataset and the default filter value might be redesigned so that it supports the non-temporal column as the default filter.
Supporting non-temporal default filters is an excellent idea! It may of course be slightly more difficult to define a globally applicable non-temporal default filter (what would the column be?), but this should at least be possible to define on the dataset level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r1000306129
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -45,6 +45,7 @@ export enum Operators {
LATEST_PARTITION = 'LATEST_PARTITION',
IS_TRUE = 'IS_TRUE',
IS_FALSE = 'IS_FALSE',
+ DATETIME_BETWEEN = 'DATETIME_BETWEEN',
Review Comment:
If I have an open ended range, like "100 years ago", then isn't it rather a "BEFORE" than a "BETWEEN"? So I would still reconsider renaming this to just "TEMPORAL" or something else instead of "BETWEEN".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple datetime filter in AdhocFilter and move away time section
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996964062
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -18,6 +18,7 @@
*/
import { buildQueryContext } from '@superset-ui/core';
import * as queryModule from '../../src/query/normalizeTimeColumn';
+import * as getXAxisModule from '../../src/query/getXAxis';
Review Comment:
The module is used to mock an object, so we have to do `import * as moduleName from 'reletive path'` statement here.
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -45,6 +45,7 @@ export enum Operators {
LATEST_PARTITION = 'LATEST_PARTITION',
IS_TRUE = 'IS_TRUE',
IS_FALSE = 'IS_FALSE',
+ DATETIME_BETWEEN = 'DATETIME_BETWEEN',
Review Comment:
Good catch, I will change it to `TEMPORAL_BETWEEN`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21767: feat: support mulitple datetime filter in AdhocFilter and move away time section
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21767:
URL: https://github.com/apache/superset/pull/21767#discussion_r996982674
##########
superset-frontend/packages/superset-ui-core/src/query/buildQueryContext.ts:
##########
@@ -48,15 +53,26 @@ export default function buildQueryContext(
? { buildQuery: options, queryFields: {} }
: options || {};
let queries = buildQuery(buildQueryObject(formData, queryFields));
+ // --- query mutator begin ---
+ // todo(Yongjie): move the query mutator into buildQueryObject instead of buildQueryContext
queries.forEach(query => {
if (Array.isArray(query.post_processing)) {
// eslint-disable-next-line no-param-reassign
query.post_processing = query.post_processing.filter(Boolean);
}
+ 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,
+ );
+ }
Review Comment:
Yes, it's for applying native filters to the ad-hoc filter on the Dashboard, this part will override all ad-hoc time filters on the Dashboard.
In the original design is that the chart only has a single time filter so 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] bilalabdullah44000 commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "bilalabdullah44000 (via GitHub)" <gi...@apache.org>.
bilalabdullah44000 commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1627532432
> > @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21767: feat: support mulitple temporal filters in AdhocFilter and move the Time Section away
Posted by "zhaoyongjie (via GitHub)" <gi...@apache.org>.
zhaoyongjie commented on PR #21767:
URL: https://github.com/apache/superset/pull/21767#issuecomment-1627109063
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org