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/04/14 08:46:12 UTC
[GitHub] [superset] villebro opened a new pull request, #19714: fix: create virtual table with exotic type
villebro opened a new pull request, #19714:
URL: https://github.com/apache/superset/pull/19714
### SUMMARY
This is an alternative implementation of #19701, which reuses the same data type inference logic that's already being used elsewhere in the codebase. While I did this fix I started adding types for the column type objects, but that turned out into a HUGE rabbit hole that ended up going well beyond the scope of this PR. Therefore the changes in this PR are kept to a minimum to fix the issue at hand, but once this is merged I'll open up a PR that introduces proper types to to these code paths.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### 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: closes #19609
- [ ] Required feature flags:
- [ ] 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] villebro merged pull request #19714: fix: create virtual table with exotic type
Posted by GitBox <gi...@apache.org>.
villebro merged PR #19714:
URL: https://github.com/apache/superset/pull/19714
--
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] sadpandajoe commented on pull request #19714: fix: create virtual table with exotic type
Posted by GitBox <gi...@apache.org>.
sadpandajoe commented on PR #19714:
URL: https://github.com/apache/superset/pull/19714#issuecomment-1101821713
🏷️ preset:2022.15
--
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 #19714: fix: create virtual table with exotic type
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19714:
URL: https://github.com/apache/superset/pull/19714#issuecomment-1098978561
# [Codecov](https://codecov.io/gh/apache/superset/pull/19714?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 [#19714](https://codecov.io/gh/apache/superset/pull/19714?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1a0268d) into [master](https://codecov.io/gh/apache/superset/commit/8865656e06e9d098d255b0da4bc3d3982b40f76d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8865656) will **decrease** coverage by `0.18%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #19714 +/- ##
==========================================
- Coverage 66.51% 66.32% -0.19%
==========================================
Files 1684 1684
Lines 64560 64553 -7
Branches 6625 6625
==========================================
- Hits 42939 42813 -126
- Misses 19926 20045 +119
Partials 1695 1695
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.95% <100.00%> (+<0.01%)` | :arrow_up: |
| postgres | `82.00% <100.00%> (+<0.01%)` | :arrow_up: |
| presto | `?` | |
| python | `82.05% <100.00%> (-0.38%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `47.75% <50.00%> (+<0.01%)` | :arrow_up: |
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/19714?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.24% <ø> (-1.22%)` | :arrow_down: |
| [superset/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3V0aWxzLnB5) | `89.01% <100.00%> (-1.81%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `70.11% <0.00%> (-15.71%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `91.89% <0.00%> (-5.41%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.64% <0.00%> (-5.39%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/result\_set.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvcmVzdWx0X3NldC5weQ==) | `96.77% <0.00%> (-1.62%)` | :arrow_down: |
| [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/19714/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-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `98.57% <0.00%> (-1.43%)` | :arrow_down: |
| ... and [6 more](https://codecov.io/gh/apache/superset/pull/19714/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19714?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19714?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8865656...1a0268d](https://codecov.io/gh/apache/superset/pull/19714?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] michael-s-molina commented on a diff in pull request #19714: fix: create virtual table with exotic type
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #19714:
URL: https://github.com/apache/superset/pull/19714#discussion_r850327848
##########
superset/connectors/sqla/utils.py:
##########
@@ -231,7 +219,7 @@ def load_or_create_tables( # pylint: disable=too-many-arguments
name=column["name"],
type=str(column["type"]),
expression=conditional_quote(column["name"]),
- is_temporal=is_column_type_temporal(column["type"]),
+ is_temporal=column["is_dttm"],
Review Comment:
The previous version checked for `date`, `datetime`, `time` and `timedelta`. Is this equivalent?
--
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 #19714: fix: create virtual table with exotic type
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #19714:
URL: https://github.com/apache/superset/pull/19714#discussion_r850227869
##########
superset/connectors/sqla/utils.py:
##########
@@ -86,7 +83,7 @@ def get_physical_table_metadata(
col.update(
{
"type": "UNKNOWN",
- "generic_type": None,
+ "type_generic": None,
Review Comment:
this was a bycatch (see line 79/76 above with the correct key) when I worked on adding proper `TypedDict`s for these objects (opening up separate PR for that one, but I thought it worthwhile to fix this error right away).
--
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 #19714: fix: create virtual table with exotic type
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #19714:
URL: https://github.com/apache/superset/pull/19714#discussion_r850345451
##########
superset/connectors/sqla/utils.py:
##########
@@ -231,7 +219,7 @@ def load_or_create_tables( # pylint: disable=too-many-arguments
name=column["name"],
type=str(column["type"]),
expression=conditional_quote(column["name"]),
- is_temporal=is_column_type_temporal(column["type"]),
+ is_temporal=column["is_dttm"],
Review Comment:
yes - this one is actually based on what the db engine spec says is temporal, so it's much more comprehensive (=can be customized per database).
--
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