You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Always-prog (via GitHub)" <gi...@apache.org> on 2023/09/06 13:31:49 UTC
[GitHub] [superset] Always-prog opened a new pull request, #25204: fix(datasource): Moved time secondary column to datasource settings
Always-prog opened a new pull request, #25204:
URL: https://github.com/apache/superset/pull/25204
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Fixes #22223.
There a feature in Superset that permanently adds a second time column to requests to Clickhouse, ElasticSearch, Kusto. But there is an problem with it, as many people think it's a bug :).
How about adding a checkbox for feature time secondary column to the dataset? i think it would be better than hardcoding it for all database connections (Clickhouse, elasticsearch, Kusto). It would also allow the ability to add a second time column to other databases. This PR doing it!
### SCREENSHOTS
<!--- Skip this if not applicable -->
### Here is how it looks for user
### 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 -->
- [x] Has associated issue: #22223
- [ ] Required feature flags:
- [x] Changes UI
- [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [x] 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] Always-prog commented on pull request #25204: feat(datasource): Checkbox for always filtering main dttm in datasource
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1710549083
@justinpark Renamed!
--
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] betodealmeida merged pull request #25204: feat(datasource): Checkbox for always filtering main dttm in datasource
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #25204:
URL: https://github.com/apache/superset/pull/25204
--
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] Always-prog commented on pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1708821935
@eschutho Currently this feature will works for all databases if checkbox is checked, and I think it's ok.
--
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] Always-prog commented on pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1710045193
@betodealmeida Done, I added unit test and fixed name of the checkbox.
--
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] eschutho commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1317563885
##########
superset-frontend/src/components/Datasource/DatasourceEditor.jsx:
##########
@@ -1003,6 +1004,16 @@ class DatasourceEditor extends React.PureComponent {
)}
control={<CheckboxControl controlId="normalize_columns" />}
/>
+ <Field
+ inline
+ fieldKey="time_secondary_column"
+ label={t('Time secondary column')}
+ description={t(
+ `When this attribute is true, wheneve the secondary dttm columns are filtered,
Review Comment:
```suggestion
`When this attribute is true, whenever the secondary dttm columns are filtered,
```
--
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] Always-prog commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1318212302
##########
tests/integration_tests/fixtures/importexport.py:
##########
@@ -496,6 +497,7 @@
"params": None,
"template_params": {},
"normalize_columns": False,
+ "time_secondary_column": False,
Review Comment:
Datasets exports and imports successfully with the `time_secondary_column`.
--
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] eschutho commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1317570072
##########
superset-frontend/src/components/Datasource/DatasourceEditor.jsx:
##########
@@ -1003,6 +1004,16 @@ class DatasourceEditor extends React.PureComponent {
)}
control={<CheckboxControl controlId="normalize_columns" />}
/>
+ <Field
+ inline
+ fieldKey="time_secondary_column"
+ label={t('Time secondary column')}
+ description={t(
+ `When this attribute is true, wheneve the secondary dttm columns are filtered,
Review Comment:
I would maybe even recommend rephrasing, to fit the syntax of the other checkboxes, something like:
`When the secondary datetime columns are filtered, apply the same filter to the main datetime column.`
--
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] eschutho commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1317567402
##########
superset/connectors/sqla/views.py:
##########
@@ -384,6 +385,12 @@ class TableModelView( # pylint: disable=too-many-ancestors
"Allow column names to be changed to case insensitive format, "
"if supported (e.g. Oracle, Snowflake)."
),
+ "time_secondary_column": _(
+ "Datasets can have a main datatime column (main_dttm_col), "
+ "but can also have secondary time columns. "
+ "When this attribute is true, wheneve the secondary columns are filtered, "
Review Comment:
```suggestion
"When this attribute is true, whenever the secondary columns are filtered, "
```
--
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] betodealmeida commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1317936488
##########
superset/connectors/sqla/views.py:
##########
@@ -384,6 +385,12 @@ class TableModelView( # pylint: disable=too-many-ancestors
"Allow column names to be changed to case insensitive format, "
"if supported (e.g. Oracle, Snowflake)."
),
+ "time_secondary_column": _(
+ "Datasets can have a main datatime column (main_dttm_col), "
+ "but can also have secondary time columns. "
+ "When this attribute is true, whenever the secondary columns are filtered, "
+ "the same filter is applied to the main datetime column."
Review Comment:
```suggestion
"Datasets can have a main temporal column (main_dttm_col), "
"but can also have secondary time columns. "
"When this attribute is true, whenever the secondary columns are filtered, "
"the same filter is applied to the main temporal column."
```
##########
superset/dashboards/schemas.py:
##########
@@ -245,6 +245,7 @@ class DashboardDatasetSchema(Schema):
time_grain_sqla = fields.List(fields.List(fields.Str()))
granularity_sqla = fields.List(fields.List(fields.Str()))
normalize_columns = fields.Bool()
+ time_secondary_column = fields.Bool()
Review Comment:
Since this is a bool, a better name might be `always_filter_primary_temporal_column`, though that's huge. Maybe `filter_primary_time_column`?
##########
superset/db_engine_specs/base.py:
##########
@@ -326,7 +326,6 @@ class BaseEngineSpec: # pylint: disable=too-many-public-methods
# Does database support join-free timeslot grouping
time_groupby_inline = False
limit_method = LimitMethod.FORCE_LIMIT
- time_secondary_columns = False
Review Comment:
Can you also update `db_engine_specs/README.md` and remove references to this attribute?
##########
superset-frontend/src/components/Datasource/DatasourceEditor.jsx:
##########
@@ -1003,6 +1004,15 @@ class DatasourceEditor extends React.PureComponent {
)}
control={<CheckboxControl controlId="normalize_columns" />}
/>
+ <Field
+ inline
+ fieldKey="time_secondary_column"
+ label={t('Time secondary column')}
+ description={t(
+ `When the secondary datetime columns are filtered, apply the same filter to the main datetime column.`,
Review Comment:
Nit: we call them "temporal" columns, since technically they don't have to be a `datetime`.
```suggestion
`When the secondary temporal columns are filtered, apply the same filter to the main temporal column.`,
```
##########
tests/integration_tests/fixtures/importexport.py:
##########
@@ -496,6 +497,7 @@
"params": None,
"template_params": {},
"normalize_columns": False,
+ "time_secondary_column": False,
Review Comment:
We need to make sure that we can still import exports that have `time_secondary_column` set.
--
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] Always-prog commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1318213756
##########
superset/dashboards/schemas.py:
##########
@@ -245,6 +245,7 @@ class DashboardDatasetSchema(Schema):
time_grain_sqla = fields.List(fields.List(fields.Str()))
granularity_sqla = fields.List(fields.List(fields.Str()))
normalize_columns = fields.Bool()
+ time_secondary_column = fields.Bool()
Review Comment:
`always_filter_main_dttm`? I'll rename it to that.
--
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] Always-prog commented on pull request #25204: feat(datasource): Checkbox for always filtering main dttm in datasource
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1715712211
@eschutho Hi! If PR is ok, can I get merge :P?
--
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] yousoph commented on pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1709235804
Suggestion on the text:
* Use header text "Secondary Time Column"
* Checkbox text: When secondary time columns are filtered, apply the same filter to the default datetime column
Should we call the field "Secondary Time Column" or "Secondary Datetime Column" to match "Default Datetime" in the datasets?
--
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] Always-prog commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1318213756
##########
superset/dashboards/schemas.py:
##########
@@ -245,6 +245,7 @@ class DashboardDatasetSchema(Schema):
time_grain_sqla = fields.List(fields.List(fields.Str()))
granularity_sqla = fields.List(fields.List(fields.Str()))
normalize_columns = fields.Bool()
+ time_secondary_column = fields.Bool()
Review Comment:
`always_filter_main_dttm`?
--
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] Always-prog commented on pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1708414911
@eschutho Hi! Should that checkbox be in the database connection? Or it need to be in the dataset settings.
--
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] justinpark commented on pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1710516107
@Always-prog This commit looks more like a feature. Could you change the title from fix to feat?
--
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] Always-prog commented on pull request #25204: feat(datasource): Checkbox for always filtering main dttm in datasource
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1711409601
@eschutho @betodealmeida Hi!
Can I get merge?
--
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] eschutho commented on a diff in pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25204:
URL: https://github.com/apache/superset/pull/25204#discussion_r1317564273
##########
superset-frontend/src/components/Datasource/DatasourceEditor.jsx:
##########
@@ -1386,6 +1397,7 @@ class DatasourceEditor extends React.PureComponent {
const { metrics } = datasource;
const sortedMetrics = metrics?.length ? this.sortMetrics(metrics) : [];
const { theme } = this.props;
+ console.log(datasource);
Review Comment:
```suggestion
```
--
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] eschutho commented on pull request #25204: fix(datasource): Moved time secondary column to datasource settings
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #25204:
URL: https://github.com/apache/superset/pull/25204#issuecomment-1708756478
@Always-prog this looks like good to me here in the dataset editor, but it wouldn't work for all databases, is that right?
cc @yousoph for another opinion.
--
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