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 2020/04/02 02:44:11 UTC
[GitHub] [incubator-superset] bkyryliuk opened a new pull request #9444:
Implement dttm column configuration through db extra config
bkyryliuk opened a new pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444
### CATEGORY
Choose one
- [ ] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Provides an ability for the superset admin to configure the date time columns per name so they will be automatically created on the table creation.
It helps a lot for the companies that have conventions & automatic day columns.
Reimplements https://github.com/apache/incubator-superset/pull/9353 to be more in line with the https://github.com/apache/incubator-superset/issues/6360
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
* unit tests
* local testing
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@john-bodley @villebro
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #9444: Implement
dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-608707947
@willbarrett and @john-bodley addressed the comments, please let me know if you have other suggestions
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066173
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
+ "default_dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
- "ds": "%Y-%m-%d"
+ "ds": "%Y-%m-%d",
+ "hour_ts": "epoch_s",
Review comment:
Nit. No trailing comma in JSON.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9444: Implement
dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-607592148
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=h1) Report
> Merging [#9444](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/3d8a2b859e567224e412f2e5e8dbb5fa83ba1d97&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9444 +/- ##
==========================================
+ Coverage 59.00% 59.02% +0.01%
==========================================
Files 378 379 +1
Lines 12166 12189 +23
Branches 3007 3014 +7
==========================================
+ Hits 7178 7194 +16
- Misses 4807 4814 +7
Partials 181 181
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/explore/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbHMuanN4) | `38.80% <0.00%> (-0.89%)` | :arrow_down: |
| [superset-frontend/src/explore/controlUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzLmpz) | `89.36% <0.00%> (-0.86%)` | :arrow_down: |
| [superset-frontend/src/chart/chartReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0UmVkdWNlci5qcw==) | `19.04% <0.00%> (ø)` | |
| [...d/src/dashboard/components/SliceHeaderControls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMuanN4) | `11.62% <0.00%> (ø)` | |
| [.../src/explore/components/ControlPanelsContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLmpzeA==) | `66.66% <0.00%> (ø)` | |
| [...et-frontend/src/components/ListView/Pagination.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvUGFnaW5hdGlvbi50c3g=) | `72.72% <0.00%> (ø)` | |
| [...dashboard/components/FilterIndicatorsContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0ZpbHRlckluZGljYXRvcnNDb250YWluZXIuanN4) | `69.35% <0.00%> (+0.50%)` | :arrow_up: |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `82.50% <0.00%> (+0.68%)` | :arrow_up: |
| [...ntend/src/dashboard/util/activeDashboardFilters.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2FjdGl2ZURhc2hib2FyZEZpbHRlcnMuanM=) | `92.30% <0.00%> (+1.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=footer). Last update [3d8a2b8...c0f973d](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066069
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
Review comment:
I sense these default settings (including the `python_date_format_by_column_name` should be documented outside of the SIP-15 section (this section can then reference said section). This would help to provide context regarding default database specific fields which may be inherited either by new Superset tables going forward or used as a fallback (in the case of SIP-15) if a value isn't defined.
Additionally I was wondering if having all these nested under a `defaults` section (and possibly scoped to domain) which ensures more consistency (and thus no need for the `default_` prefix), i.e., something of the form:
```json
{
"defaults": {
"table": {
"main_dttm_col": "ds",
"expression_by_column_name": {
"hour_ts": "CAST(hour_ts as INTEGER)"
},
"dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
"ds": "%Y-%m-%d",
"hour_ts": "epoch_s"
}
}
}
}
```
Note if we go down this route the `UPDATING.md` would need to be updated to reflect the breaking SIP-15 config change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066069
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
Review comment:
I sense these default settings (including the `python_date_format_by_column_name` should be documented outside of the SIP-15 section (this section can then reference said section). This would help to provide context regarding default database specific fields which may be inherited either by new Superset tables going forward or used as a fallback (in the case of SIP-15) if a value isn't defined.
Additionally I was wondering if having all these nested under a `defaults` section and scoped to tables which ensures more consistency (and thus no need for the `default_` prefix), i.e., something of the form:
```json
{
"defaults": {
"tables": {
"main_dttm_col": "ds",
"expression_by_column_name": {
"hour_ts": "CAST(hour_ts as INTEGER)"
},
"dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
"ds": "%Y-%m-%d",
"hour_ts": "epoch_s"
}
}
}
}
```
Note if we go down this route the `UPDATING.md` would need to be updated to reflect the breaking SIP-15 config change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066069
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
Review comment:
I sense these default settings (including the `python_date_format_by_column_name` should be documented outside of the SIP-15 section (this section can then reference said section). This would help to provide context regarding default database specific fields which may be inherited either by new Superset tables going forward or used as a fallback (in the case of SIP-15) if a value isn't defined.
Additionally I was wondering if having all these nested under a `defaults` section ensures more consistency (and thus no need for the `default_` prefix),
```json
{
"defaults": {
"dttm_column_names": ["ds", "hour_ts"],
"main_dttm_col": "ds",
"expression_by_column_name": {
"hour_ts": "CAST(hour_ts as INTEGER)"
},
"python_date_format_by_column_name": {
"ds": "%Y-%m-%d",
"hour_ts": "epoch_s"
}
}
}
```
would be better documented in their own section, i.e., outside of SIP-15. It could then be clearer to the reader that these database level overrides
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r405247136
##########
File path: superset/connectors/sqla/models.py
##########
@@ -145,6 +147,26 @@ class TableColumn(Model, BaseColumn):
update_from_object_fields = [s for s in export_fields if s not in ("table_id",)]
export_parent = "table"
+ def get_expression(self):
+ if config["DEFAULT_DB_DTTM_ENABLED"] and not self.expression:
+ # TODO(bkyryliuk): consider serializing defaults to the DB for better visibility.
Review comment:
yes in the near future if there are no objections
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9444:
Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-607592148
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=h1) Report
> Merging [#9444](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/3d8a2b859e567224e412f2e5e8dbb5fa83ba1d97&el=desc) will **increase** coverage by `11.08%`.
> The diff coverage is `81.57%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9444 +/- ##
===========================================
+ Coverage 59.00% 70.08% +11.08%
===========================================
Files 378 182 -196
Lines 12166 17539 +5373
Branches 3007 0 -3007
===========================================
+ Hits 7178 12293 +5115
- Misses 4807 5246 +439
+ Partials 181 0 -181
```
| Flag | Coverage Δ | |
|---|---|---|
| #python | `70.08% <81.57%> (?)` | |
| #sqlite | `70.08% <81.57%> (?)` | |
| #unittest | `70.08% <81.57%> (?)` | |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `87.31% <79.41%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `89.53% <100.00%> (ø)` | |
| [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `84.56% <100.00%> (ø)` | |
| [...t-frontend/src/dashboard/containers/SliceAdder.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL1NsaWNlQWRkZXIuanN4) | | |
| [superset-frontend/src/components/ModalTrigger.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTW9kYWxUcmlnZ2VyLmpzeA==) | | |
| [...c/dashboard/components/gridComponents/Markdown.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL01hcmtkb3duLmpzeA==) | | |
| [superset-frontend/src/showSavedQuery/utils.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Nob3dTYXZlZFF1ZXJ5L3V0aWxzLmpz) | | |
| [.../src/explore/components/controls/SliderControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TbGlkZXJDb250cm9sLmpzeA==) | | |
| [...d/src/dashboard/util/updateComponentParentsList.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL3VwZGF0ZUNvbXBvbmVudFBhcmVudHNMaXN0Lmpz) | | |
| [superset-frontend/src/modules/AnnotationTypes.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL21vZHVsZXMvQW5ub3RhdGlvblR5cGVzLmpz) | | |
| ... and [553 more](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=footer). Last update [3d8a2b8...00dbf78](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #9444: Implement
dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-610740991
@willbarrett addressed the comments, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r405159196
##########
File path: superset/connectors/sqla/models.py
##########
@@ -145,6 +147,26 @@ class TableColumn(Model, BaseColumn):
update_from_object_fields = [s for s in export_fields if s not in ("table_id",)]
export_parent = "table"
+ def get_expression(self):
+ if config["DEFAULT_DB_DTTM_ENABLED"] and not self.expression:
+ # TODO(bkyryliuk): consider serializing defaults to the DB for better visibility.
Review comment:
Is this TODO going to be handled in the near future? If not, I'd recommend removing it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r403169620
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
+ "default_dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
- "ds": "%Y-%m-%d"
+ "ds": "%Y-%m-%d",
+ "hour_ts": "epoch_s",
+ }
+ "expression_by_column_name": {
+ "hour_ts": "CAST(hour_ts as INTEGER)",
Review comment:
@john-bodley we actually use both of them in combination - this is a real example from our setup.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r405158686
##########
File path: docs/installation.rst
##########
@@ -1341,6 +1341,32 @@ Then we can add this two lines to ``superset_config.py``:
from custom_sso_security_manager import CustomSsoSecurityManager
CUSTOM_SECURITY_MANAGER = CustomSsoSecurityManager
+Default date time column settings
+----------------------------------
+In some cases there are a company wide convention for the date time columns.
+Now it is possible to provide a default column mapping on a per database level via the ``extra`` parameter.
+If no formatting was defined - it will fallback to the database defaults.
Review comment:
Unnecessary `-`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066069
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
Review comment:
I sense these default settings (including the `python_date_format_by_column_name` should be documented outside of the SIP-15 section (this section can then reference said section). This would help to provide context regarding default database specific fields which may be inherited either by new Superset tables going forward or used as a fallback (in the case of SIP-15) if a value isn't defined.
Additionally I was wondering if having all these nested under a `defaults` section ensures more consistency (and thus no need for the `default_` prefix),
```json
{
"defaults": {
"main_dttm_col": "ds",
"expression_by_column_name": {
"hour_ts": "CAST(hour_ts as INTEGER)"
},
"dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
"ds": "%Y-%m-%d",
"hour_ts": "epoch_s"
}
}
}
```
would be better documented in their own section, i.e., outside of SIP-15. It could then be clearer to the reader that these database level overrides. If we go down this route the `UPDATING.md` would need to be updated to reflect the breaking SIP-15 config change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402460714
##########
File path: tests/core_tests.py
##########
@@ -1171,6 +1171,78 @@ def test_sqllab_backend_persistence_payload(self):
payload = views.Superset._get_sqllab_tabs(user_id=user_id)
self.assertEqual(len(payload["queries"]), 1)
+ def test_db_column_defaults(self):
Review comment:
Can we break down this test?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9444:
Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-614326826
No further comments from me. I recommend pinging a couple more contributors to review.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066069
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
Review comment:
I sense these default settings (including the `python_date_format_by_column_name` should be documented outside of the SIP-15 section (this section can then reference said section). This would help to provide context regarding default database specific fields which may be inherited either by new Superset tables going forward or used as a fallback (in the case of SIP-15) if a value isn't defined.
Additionally I was wondering if having all these nested under a `defaults` section and scoped to tables which ensures more consistency (and thus no need for the `default_` prefix), i.e., something of the form:
```json
{
"defaults": {
"table": {
"main_dttm_col": "ds",
"expression_by_column_name": {
"hour_ts": "CAST(hour_ts as INTEGER)"
},
"dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
"ds": "%Y-%m-%d",
"hour_ts": "epoch_s"
}
}
}
}
```
Note if we go down this route the `UPDATING.md` would need to be updated to reflect the breaking SIP-15 config change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402511181
##########
File path: superset/connectors/sqla/models.py
##########
@@ -145,6 +146,17 @@ class TableColumn(Model, BaseColumn):
update_from_object_fields = [s for s in export_fields if s not in ("table_id",)]
export_parent = "table"
+ def get_expression(self):
+ if config["SIP_15_ENABLED"] and not self.expression:
+ db_default_expression = (
+ self.table.database.get_extra()
+ .get("expression_by_column_name", {})
+ .get(self.column_name)
+ )
+ # TODO(bkyryliuk): consider setting self.expression to db_default_expreassion
Review comment:
nit: typo `db_default_expreassion`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402070128
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
+ "default_dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
- "ds": "%Y-%m-%d"
+ "ds": "%Y-%m-%d",
+ "hour_ts": "epoch_s",
+ }
+ "expression_by_column_name": {
+ "hour_ts": "CAST(hour_ts as INTEGER)",
Review comment:
I think this example is a tad misleading as you woudn't have the same column name using both the Python datetime format and expression as these are mutually exclusive.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r405247136
##########
File path: superset/connectors/sqla/models.py
##########
@@ -145,6 +147,26 @@ class TableColumn(Model, BaseColumn):
update_from_object_fields = [s for s in export_fields if s not in ("table_id",)]
export_parent = "table"
+ def get_expression(self):
+ if config["DEFAULT_DB_DTTM_ENABLED"] and not self.expression:
+ # TODO(bkyryliuk): consider serializing defaults to the DB for better visibility.
Review comment:
yes in near future if there are no objections
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9444:
Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-607943599
@bkyryliuk one other question I had, and maybe the community should weigh in here, is whether i) defaults should be persisted on tables etc. or ii) whether undefined values should fallback to the defaults.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402062452
##########
File path: superset/connectors/sqla/models.py
##########
@@ -145,6 +146,17 @@ class TableColumn(Model, BaseColumn):
update_from_object_fields = [s for s in export_fields if s not in ("table_id",)]
export_parent = "table"
+ def get_expression(self):
+ if config["SIP_15_ENABLED"] and not self.expression:
+ db_default_expression = (
Review comment:
Why not just `return ( ...`. This results in one less variable, etc. and thus make the code easier to grok.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402061395
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
Review comment:
I wonder if this should be named `main_dttm_col` to be consistent with the datasource level field.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #9444: Implement
dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-613168211
@willbarrett , @john-bodley - is there anything else I can do to get this merged?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402070128
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
+ "default_dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
- "ds": "%Y-%m-%d"
+ "ds": "%Y-%m-%d",
+ "hour_ts": "epoch_s",
+ }
+ "expression_by_column_name": {
+ "hour_ts": "CAST(hour_ts as INTEGER)",
Review comment:
I think this example is a tad misleading as you wouldn't have the same column name using both the Python datetime format and expression as these are mutually exclusive.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk merged pull request #9444: feat: implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk merged pull request #9444:
URL: https://github.com/apache/incubator-superset/pull/9444
----------------------------------------------------------------
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.
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] [incubator-superset] codecov-io edited a comment on issue #9444:
Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-607592148
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=h1) Report
> Merging [#9444](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/3d8a2b859e567224e412f2e5e8dbb5fa83ba1d97&el=desc) will **decrease** coverage by `0.23%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9444 +/- ##
==========================================
- Coverage 59.00% 58.76% -0.24%
==========================================
Files 378 385 +7
Lines 12166 12237 +71
Branches 3007 3020 +13
==========================================
+ Hits 7178 7191 +13
- Misses 4807 4862 +55
- Partials 181 184 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/explore/controlUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzLmpz) | `89.36% <0.00%> (-0.86%)` | :arrow_down: |
| [superset-frontend/src/chart/chartReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0UmVkdWNlci5qcw==) | `19.04% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `35.39% <0.00%> (ø)` | |
| [...et-frontend/src/explore/controlPanels/sections.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9zZWN0aW9ucy5qc3g=) | `100.00% <0.00%> (ø)` | |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `25.00% <0.00%> (ø)` | |
| [...-frontend/src/explore/controlPanels/DeckScatter.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9EZWNrU2NhdHRlci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...-frontend/src/explore/reducers/saveModalReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvc2F2ZU1vZGFsUmVkdWNlci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...rontend/src/visualizations/FilterBox/FilterBox.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL0ZpbHRlckJveC9GaWx0ZXJCb3guanN4) | `5.08% <0.00%> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <0.00%> (ø)` | |
| [...d/src/dashboard/components/SliceHeaderControls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1NsaWNlSGVhZGVyQ29udHJvbHMuanN4) | `11.62% <0.00%> (ø)` | |
| ... and [15 more](https://codecov.io/gh/apache/incubator-superset/pull/9444/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=footer). Last update [3d8a2b8...77d0244](https://codecov.io/gh/apache/incubator-superset/pull/9444?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402461099
##########
File path: tests/core_tests.py
##########
@@ -1171,6 +1171,78 @@ def test_sqllab_backend_persistence_payload(self):
payload = views.Superset._get_sqllab_tabs(user_id=user_id)
self.assertEqual(len(payload["queries"]), 1)
+ def test_db_column_defaults(self):
+ db_extras = json.dumps(
+ {
+ "main_datetime_column": "dttm",
+ "default_dttm_column_names": ["id", "dttm"],
+ "python_date_format_by_column_name": {
+ "id": "epoch_ms",
+ "dttm": "epoch_s",
+ },
+ "expression_by_column_name": {"dttm": "CAST(dttm as INTEGER)"},
+ }
+ )
+
+ self.login(username="admin")
+ try:
+ test_db = utils.get_or_create_db(
+ "column_test_db", app.config["SQLALCHEMY_DATABASE_URI"], extra=db_extras
+ )
+
+ resp = self.client.post(
+ "/tablemodelview/add",
+ data=dict(database=test_db.id, table_name="logs"),
+ follow_redirects=True,
+ )
+ self.assertEqual(resp.status_code, 200)
+ added_table = db.session.query(SqlaTable).filter_by(table_name="logs").one()
+
+ # Make sure that dttm column is set properly
Review comment:
Comments like this in a test file usually indicate to me that the test should be broken up, and that the comments are a good place to do the breaking.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9444: Implement dttm column configuration through db extra config
URL: https://github.com/apache/incubator-superset/pull/9444#discussion_r402066155
##########
File path: docs/installation.rst
##########
@@ -1379,8 +1379,14 @@ Prior to SIP-15 SQLAlchemy used inclusive endpoints however these may behave lik
To remedy this rather than having to define the date/time format for every non-IS0 8601 date-time column, once can define a default column mapping on a per database level via the ``extra`` parameter ::
{
+ "main_dttm_column": "ds",
+ "default_dttm_column_names": ["ds", "hour_ts"],
"python_date_format_by_column_name": {
- "ds": "%Y-%m-%d"
+ "ds": "%Y-%m-%d",
+ "hour_ts": "epoch_s",
+ }
+ "expression_by_column_name": {
+ "hour_ts": "CAST(hour_ts as INTEGER)",
Review comment:
Nit. No trailing comma in JSON.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] bkyryliuk commented on issue #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-618429802
> https://github.com/apache/incubator-superset/compare/master...mistercrunch:example_config?expand=1
I like it, will redo the 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.
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] [incubator-superset] mistercrunch commented on issue #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-618212323
Here's an example of what I meant:
https://github.com/apache/incubator-superset/compare/master...mistercrunch:example_config?expand=1
Using this, you can implement whatever logic to change table configuration based on naming conventions and such.
Would that work for 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.
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] [incubator-superset] john-bodley commented on pull request #9444: feat: implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-645754058
@bkyryliuk do you think there is merit in addressing [this](https://github.com/apache/incubator-superset/pull/9444#discussion_r402066069) comment in a follow up PR? I'm a little concerned that there's two different mechanism for defining the Python date-format within the extra blob.
----------------------------------------------------------------
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.
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] [incubator-superset] bkyryliuk commented on issue #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-618076196
> @bkyryliuk I have no further blockers, but I:
>
> 1. Don't understand this part of the system well enough to give you a merge approval
> 2. Am not a commiter, so my approval carries little weight.
>
> Again, please reach out to additional committers to see if this is viable. It sounds like @mistercrunch would prefer an alternate solution.
Thanks for the context. Will do.
----------------------------------------------------------------
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.
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] [incubator-superset] bkyryliuk commented on issue #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on issue #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-617340991
Thanks @mistercrunch , I think as an alternative, we could benefit from making them high level db configuration in the ui. It would make things more transparent. However this would be probably out of scope of this PR.
@willbarrett is PR in a good state to 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.
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] [incubator-superset] bkyryliuk commented on pull request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-620822967
test failure doesn't seem to be releated.
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on issue #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-616884502
Generally LGTM
I think personally I prefer dynamic configuration hooks, where you'd define a function that receives a live table object upon insert and/or modify, and mutate it with whatever business/semantical logic make sense to your org, keeping all the logic on your side.
Intricate static configuration like the one defined here is hard to discover, often requires to read code to understand what it does / can do, and often has shortcomings / requires code change to the core package to get things done.
----------------------------------------------------------------
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.
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] [incubator-superset] bkyryliuk commented on pull request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-618762495
@mistercrunch - updated the PR to follow the mutator pattern
----------------------------------------------------------------
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.
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] [incubator-superset] willbarrett commented on issue #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-618005299
@bkyryliuk I have no further blockers, but I:
1. Don't understand this part of the system well enough to give you a merge approval
2. Am not a commiter, so my approval carries little weight.
Again, please reach out to additional committers to see if this is viable. It sounds like @mistercrunch would prefer an alternate solution.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #9444: Implement dttm column configuration through db extra config
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #9444:
URL: https://github.com/apache/incubator-superset/pull/9444#issuecomment-620958510
@bkyryliuk I restarted CI, I had a similar problem yesterday.
----------------------------------------------------------------
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.
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