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 2021/02/05 01:48:32 UTC
[GitHub] [superset] etr2460 opened a new pull request #12960: fix: Retroactively add granularity param to charts
etr2460 opened a new pull request #12960:
URL: https://github.com/apache/superset/pull/12960
### SUMMARY
We ran into some data accuracy issues in our environment where time range filters wouldn't apply to certain charts (like Big Number) when set on a dashboard. We determined that this was because these charts somehow didn't have a granularity param set on them, so the time range wasn't appropriately set on the chart query. This was especially dangerous, because the chart showed as applying the filter, but it wasn't actually applied to the query.
The fix is this db migration that adds the granularity param onto charts that should have one but don't.
### TEST PLAN
Before the migration, see big number charts without the granularity or granularity_sqla param not filter by time range correctly.
Run the migration, and see the dashboard perform the filter appropriately.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [x] Requires DB Migration.
- [x] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
to: @john-bodley @ktmud @graceguo-supercat @villebro
cc: @junlincc
----------------------------------------------------------------
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] [superset] etr2460 commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r570715227
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
Review comment:
good call, 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] [superset] ktmud commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r570850468
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,126 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one (this
+ matches the behavior of Explore view on the frontend)
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
Review comment:
This query will fetch all chart slices with SQLA datasource and run JSON parse in Python. Could we filter out only those didn't have `granularity` or `granularity_sqla` instead?
```suggestion
for slc in (
session.query(Slice)
.filter(and_(
Slice.datasource_type == "table",
not Slice.params.like('%"granularity%'))
))
.yield_per(500)
):
```
Not that matters in practicality, but I'd also try to stream things whenever I try to fetch an unknown number of all results (`.yield_per(500)`).
----------------------------------------------------------------
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] [superset] ktmud commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r571187473
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,126 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one (this
+ matches the behavior of Explore view on the frontend)
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
Review comment:
Yeah, since we don't need complex regexp here so a simple text match should work.
----------------------------------------------------------------
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] [superset] ktmud commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-776921997
Close and reopen to trigger `docker-build` and `python-lint (3.7)`:
----------------------------------------------------------------
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] [superset] villebro commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773848587
@etr2460 I noticed a similar weirdness when I was fixing a regression in the table chart. For some reason the chart looked different in the dashboard compared to the Explore view. When looking at the metadata I noticed that the chart was missing a control value that had been added to the control panel after the chart had been created. Upon closer inspection it turned out Explore merges the chart metadata on top of the default control values, but dashboard doesn't. I didn't yet have time to look into this more closely, but I believe making sure the metadata flow is the same in Dashboard and Explore view might solve this problem, potentially making the migration 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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
----------------------------------------------------------------
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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `0.28%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 68.85% -0.29%
==========================================
Files 1025 1027 +2
Lines 48767 48871 +104
Branches 5188 5199 +11
==========================================
- Hits 33718 33652 -66
- Misses 14915 15084 +169
- Partials 134 135 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.82% <ø> (-0.01%)` | :arrow_down: |
| javascript | `61.79% <ø> (-0.05%)` | :arrow_down: |
| python | `67.16% <0.00%> (-0.46%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-16.93%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.38% <0.00%> (-6.50%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <0.00%> (-4.22%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <0.00%> (-3.29%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.04% <0.00%> (-0.82%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `85.50% <0.00%> (-0.50%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r570714885
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
+ try:
+ params = json.loads(slc.params)
+ if "granularity" in params or "granularity_sqla" in params:
+ continue
+
+ table = session.query(SqlaTable).get(slc.datasource_id)
Review comment:
This ended up only altering 150 slices in our DB (and only got to this step for about 4k) so i'm not sure performance matters that much. It's a trade off between doing the join with a much larger number of slices (200k+) vs. waiting until we get to this step. idk
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
Review comment:
good call, 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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (30660ce) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `2.43%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 66.70% -2.44%
==========================================
Files 1025 491 -534
Lines 48767 28888 -19879
Branches 5188 0 -5188
==========================================
- Hits 33718 19269 -14449
+ Misses 14915 9619 -5296
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `66.70% <0.00%> (-0.92%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/views/database/views.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2Uvdmlld3MucHk=) | `62.69% <0.00%> (-24.88%)` | :arrow_down: |
| [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `83.67% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/sql\_validators/base.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvYmFzZS5weQ==) | `93.33% <0.00%> (-6.67%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `79.85% <0.00%> (-6.15%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `82.25% <0.00%> (-5.63%)` | :arrow_down: |
| ... and [577 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...30660ce](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `0.33%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 68.80% -0.34%
==========================================
Files 1025 1027 +2
Lines 48767 48871 +104
Branches 5188 5199 +11
==========================================
- Hits 33718 33626 -92
- Misses 14915 15110 +195
- Partials 134 135 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.82% <ø> (-0.01%)` | :arrow_down: |
| javascript | `61.79% <ø> (-0.05%)` | :arrow_down: |
| python | `67.07% <0.00%> (-0.55%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-16.93%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.38% <0.00%> (-6.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <0.00%> (-4.22%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <0.00%> (-3.29%)` | :arrow_down: |
| ... and [25 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (9c6ba17) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `1.32%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 67.81% -1.33%
==========================================
Files 1025 490 -535
Lines 48767 29369 -19398
Branches 5188 0 -5188
==========================================
- Hits 33718 19918 -13800
+ Misses 14915 9451 -5464
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `67.81% <0.00%> (+0.19%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-16.93%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `94.73% <0.00%> (-5.27%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/dashboards/commands/update.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy91cGRhdGUucHk=) | `85.50% <0.00%> (-3.18%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `89.79% <0.00%> (-2.05%)` | :arrow_down: |
| [superset/result\_set.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVzdWx0X3NldC5weQ==) | `96.69% <0.00%> (-1.66%)` | :arrow_down: |
| ... and [560 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...9c6ba17](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-775038150
> @etr2460 I noticed a similar weirdness when I was fixing a regression in the table chart. For some reason the chart looked different in the dashboard compared to the Explore view. When looking at the metadata I noticed that the chart was missing a control value that had been added to the control panel after the chart had been created. Upon closer inspection it turned out Explore merges the chart metadata on top of the default control values, but dashboard doesn't. I didn't yet have time to look into this more closely, but I believe making sure the metadata flow is the same in Dashboard and Explore view might solve this problem, potentially making the migration unnecessary.
I looked into this, and it turns out default values are in fact applied on the Chart form data on the Dashboard similarly as on the Explore view. The control panel on the Table chart was just setting the value of the `queryModel` control (which had the default value `null`) while rendering the control panel based on other form data.
Going forward we should potentially make default values "smarter", by making it possible to introduce hooks that return defaults based on other context. In the case of `queryMode` in table model, it would be inferred from the other `formData`, and in the case of granularity, it could default to `main_dttm_col` in the dataset if missing/unset.
----------------------------------------------------------------
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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `18.68%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
===========================================
- Coverage 69.14% 50.45% -18.69%
===========================================
Files 1025 478 -547
Lines 48767 17243 -31524
Branches 5188 4453 -735
===========================================
- Hits 33718 8700 -25018
+ Misses 14915 8543 -6372
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.45% <ø> (-0.38%)` | :arrow_down: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/nativeFilters/ScopingTree.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvU2NvcGluZ1RyZWUudHN4) | `6.25% <0.00%> (-93.75%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...perset-frontend/src/views/CRUD/welcome/Welcome.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9XZWxjb21lLnRzeA==) | `2.94% <0.00%> (-85.95%)` | :arrow_down: |
| ... and [884 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud closed pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
ktmud closed pull request #12960:
URL: https://github.com/apache/superset/pull/12960
----------------------------------------------------------------
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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `0.30%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 68.84% -0.31%
==========================================
Files 1025 1027 +2
Lines 48767 48871 +104
Branches 5188 5199 +11
==========================================
- Hits 33718 33643 -75
- Misses 14915 15093 +178
- Partials 134 135 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.82% <ø> (-0.01%)` | :arrow_down: |
| javascript | `61.79% <ø> (-0.05%)` | :arrow_down: |
| python | `67.13% <0.00%> (-0.49%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-16.93%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.38% <0.00%> (-6.50%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <0.00%> (-4.22%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <0.00%> (-3.29%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| ... and [22 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `18.31%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
===========================================
- Coverage 69.14% 50.82% -18.32%
===========================================
Files 1025 478 -547
Lines 48767 17243 -31524
Branches 5188 4453 -735
===========================================
- Hits 33718 8764 -24954
+ Misses 14915 8479 -6436
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.82% <ø> (-0.01%)` | :arrow_down: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/nativeFilters/ScopingTree.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvU2NvcGluZ1RyZWUudHN4) | `6.25% <0.00%> (-93.75%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...perset-frontend/src/views/CRUD/welcome/Welcome.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9XZWxjb21lLnRzeA==) | `2.94% <0.00%> (-85.95%)` | :arrow_down: |
| ... and [884 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (9c6ba17) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `6.64%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 62.49% -6.65%
==========================================
Files 1025 979 -46
Lines 48767 46684 -2083
Branches 5188 4482 -706
==========================================
- Hits 33718 29176 -4542
- Misses 14915 17508 +2593
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `53.46% <ø> (+2.63%)` | :arrow_up: |
| javascript | `?` | |
| python | `67.81% <0.00%> (+0.19%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...perset-frontend/src/views/CRUD/welcome/Welcome.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9XZWxjb21lLnRzeA==) | `2.94% <0.00%> (-85.95%)` | :arrow_down: |
| ... and [451 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...9c6ba17](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r570671576
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
Review comment:
Should this mention that this mimics the behavior of the frontend?
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
+ try:
+ params = json.loads(slc.params)
+ if "granularity" in params or "granularity_sqla" in params:
+ continue
+
+ table = session.query(SqlaTable).get(slc.datasource_id)
Review comment:
Is this performant? I wonder if the join should be part of the slice query.
----------------------------------------------------------------
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] [superset] etr2460 merged pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
etr2460 merged pull request #12960:
URL: https://github.com/apache/superset/pull/12960
----------------------------------------------------------------
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] [superset] etr2460 commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-777619794
good call, updating now. And I agree @villebro there's some weirdness going on with passing parameters the same way between dashboard and explore. This might not be a long term fix, but more so fixes weirdness that's been cropping up currently
----------------------------------------------------------------
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] [superset] villebro commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773848587
@etr2460 I noticed a similar weirdness when I was fixing a regression in the table chart. For some reason the chart looked different in the dashboard compared to the Explore view. When looking at the metadata I noticed that the chart was missing a control value that had been added to the control panel after the chart had been created. Upon closer inspection it turned out Explore merges the chart metadata on top of the default control values, but dashboard doesn't. I didn't yet have time to look into this more closely, but I believe making sure the metadata flow is the same in Dashboard and Explore view might solve this problem, potentially making the migration 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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (9c6ba17) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `6.78%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 62.35% -6.79%
==========================================
Files 1025 979 -46
Lines 48767 46684 -2083
Branches 5188 4482 -706
==========================================
- Hits 33718 29112 -4606
- Misses 14915 17572 +2657
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `53.09% <ø> (+2.26%)` | :arrow_up: |
| javascript | `?` | |
| python | `67.81% <0.00%> (+0.19%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...perset-frontend/src/views/CRUD/welcome/Welcome.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvd2VsY29tZS9XZWxjb21lLnRzeA==) | `2.94% <0.00%> (-85.95%)` | :arrow_down: |
| ... and [451 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...9c6ba17](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov-io commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `20.34%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
===========================================
- Coverage 69.14% 48.79% -20.35%
===========================================
Files 1025 478 -547
Lines 48767 17243 -31524
Branches 5188 4453 -735
===========================================
- Hits 33718 8414 -25304
+ Misses 14915 8829 -6086
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `48.79% <ø> (-2.04%)` | :arrow_down: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...erset-frontend/src/common/hooks/useChangeEffect.ts](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9ob29rcy91c2VDaGFuZ2VFZmZlY3QudHM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/nativeFilters/ScopingTree.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvU2NvcGluZ1RyZWUudHN4) | `6.25% <0.00%> (-93.75%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| ... and [885 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-777575756
FYI a migration was merged today, so I believe we need to update the downgrade revision id. https://github.com/apache/superset/pull/13052
----------------------------------------------------------------
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] [superset] john-bodley commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r570671576
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
Review comment:
Should this mention that this mimics the behavior of the frontend?
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
+ try:
+ params = json.loads(slc.params)
+ if "granularity" in params or "granularity_sqla" in params:
+ continue
+
+ table = session.query(SqlaTable).get(slc.datasource_id)
Review comment:
Is this performant? I wonder if the join should be part of the slice query.
----------------------------------------------------------------
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] [superset] codecov-io commented on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `20.34%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
===========================================
- Coverage 69.14% 48.79% -20.35%
===========================================
Files 1025 478 -547
Lines 48767 17243 -31524
Branches 5188 4453 -735
===========================================
- Hits 33718 8414 -25304
+ Misses 14915 8829 -6086
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `48.79% <ø> (-2.04%)` | :arrow_down: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...erset-frontend/src/common/hooks/useChangeEffect.ts](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9ob29rcy91c2VDaGFuZ2VFZmZlY3QudHM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/nativeFilters/ScopingTree.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvU2NvcGluZ1RyZWUudHN4) | `6.25% <0.00%> (-93.75%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| ... and [885 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r570714885
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,125 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one.
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
+ try:
+ params = json.loads(slc.params)
+ if "granularity" in params or "granularity_sqla" in params:
+ continue
+
+ table = session.query(SqlaTable).get(slc.datasource_id)
Review comment:
This ended up only altering 150 slices in our DB (and only got to this step for about 4k) so i'm not sure performance matters that much. It's a trade off between doing the join with a much larger number of slices (200k+) vs. waiting until we get to this step. idk
----------------------------------------------------------------
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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (9c6ba17) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `2.30%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 66.83% -2.31%
==========================================
Files 1025 490 -535
Lines 48767 28772 -19995
Branches 5188 0 -5188
==========================================
- Hits 33718 19230 -14488
+ Misses 14915 9542 -5373
+ Partials 134 0 -134
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `66.83% <0.00%> (-0.79%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-16.93%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `83.67% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `90.62% <0.00%> (-6.25%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `84.51% <0.00%> (-6.04%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `84.78% <0.00%> (-4.35%)` | :arrow_down: |
| ... and [557 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...9c6ba17](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] etr2460 commented on a change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r571098628
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,126 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one (this
+ matches the behavior of Explore view on the frontend)
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
Review comment:
so you're saying do a plain text filter first to remove most of the slices that aren't eligible, and only do the json parse on what's remaining? It looks janky, but it should work, will update
----------------------------------------------------------------
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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **increase** coverage by `2.13%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
+ Coverage 69.14% 71.27% +2.13%
==========================================
Files 1025 537 -488
Lines 48767 20125 -28642
Branches 5188 5199 +11
==========================================
- Hits 33718 14345 -19373
+ Misses 14915 5645 -9270
- Partials 134 135 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.82% <ø> (-0.01%)` | :arrow_down: |
| javascript | `61.79% <ø> (-0.05%)` | :arrow_down: |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <0.00%> (-4.22%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <0.00%> (-3.29%)` | :arrow_down: |
| [...ontend/src/dashboard/components/dnd/handleHover.js](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2RuZC9oYW5kbGVIb3Zlci5qcw==) | `100.00% <0.00%> (ø)` | |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | | |
| [...tions/versions/e9df189e5c7e\_update\_base\_metrics.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9lOWRmMTg5ZTVjN2VfdXBkYXRlX2Jhc2VfbWV0cmljcy5weQ==) | | |
| [superset/views/css\_templates.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY3NzX3RlbXBsYXRlcy5weQ==) | | |
| [...t/annotation\_layers/annotations/commands/delete.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvYW5ub3RhdGlvbnMvY29tbWFuZHMvZGVsZXRlLnB5) | | |
| [superset/migrations/versions/7fcdcde0761c\_.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy83ZmNkY2RlMDc2MWNfLnB5) | | |
| [.../migrations/versions/c617da68de7d\_form\_nullable.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy9jNjE3ZGE2OGRlN2RfZm9ybV9udWxsYWJsZS5weQ==) | | |
| [superset/databases/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2ltcG9ydGVycy92MS91dGlscy5weQ==) | | |
| ... and [484 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
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 change in pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #12960:
URL: https://github.com/apache/superset/pull/12960#discussion_r571101902
##########
File path: superset/migrations/versions/070c043f2fdb_add_granularity_to_charts_where_missing.py
##########
@@ -0,0 +1,126 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add granularity to charts where missing
+
+Revision ID: 070c043f2fdb
+Revises: e11ccdd12658
+Create Date: 2021-02-04 09:34:13.608891
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "070c043f2fdb"
+down_revision = "e11ccdd12658"
+
+import json
+
+from alembic import op
+from sqlalchemy import Boolean, Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+
+Base = declarative_base()
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ params = Column(Text)
+ datasource_id = Column(Integer)
+ datasource_type = Column(String(200))
+
+
+class SqlaTable(Base):
+ __tablename__ = "tables"
+
+ id = Column(Integer, primary_key=True)
+ main_dttm_col = Column(String(250))
+
+
+class TableColumn(Base):
+ __tablename__ = "table_columns"
+
+ id = Column(Integer, primary_key=True)
+ table_id = Column(Integer)
+ is_dttm = Column(Boolean)
+ column_name = Column(String(255))
+
+
+def upgrade():
+ """
+ Adds the granularity param to charts without it populated. This is required for
+ time range filtering to work properly. Uses the following approach:
+
+ - Find all charts without a granularity or granularity_sqla param.
+ - Get the dataset that backs the chart.
+ - If the dataset has the main dttm column set, use it.
+ - Otherwise, find all the dttm columns in the dataset and use the first one (this
+ matches the behavior of Explore view on the frontend)
+ - If no dttm columns exist in the dataset, don't change the chart.
+ """
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices_changed = 0
+
+ for slc in session.query(Slice).filter(Slice.datasource_type == "table").all():
Review comment:
I think this makes sense - no point in pulling in slices that aren't applicable
----------------------------------------------------------------
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] [superset] codecov-io edited a comment on pull request #12960: fix: Retroactively add granularity param to charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12960:
URL: https://github.com/apache/superset/pull/12960#issuecomment-773786764
# [Codecov](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=h1) Report
> Merging [#12960](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=desc) (12210ca) into [master](https://codecov.io/gh/apache/superset/commit/9982fdebd8a312251e3e1821324903efe7e33c06?el=desc) (9982fde) will **decrease** coverage by `0.26%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12960 +/- ##
==========================================
- Coverage 69.14% 68.87% -0.27%
==========================================
Files 1025 1027 +2
Lines 48767 48899 +132
Branches 5188 5199 +11
==========================================
- Hits 33718 33679 -39
- Misses 14915 15085 +170
- Partials 134 135 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `50.82% <ø> (-0.01%)` | :arrow_down: |
| javascript | `61.79% <ø> (-0.05%)` | :arrow_down: |
| python | `67.19% <0.00%> (-0.43%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12960?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-16.93%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.38% <0.00%> (-6.50%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <0.00%> (-4.22%)` | :arrow_down: |
| [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <0.00%> (-3.29%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.04% <0.00%> (-0.82%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `85.50% <0.00%> (-0.50%)` | :arrow_down: |
| ... and [12 more](https://codecov.io/gh/apache/superset/pull/12960/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12960?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/superset/pull/12960?src=pr&el=footer). Last update [9982fde...12210ca](https://codecov.io/gh/apache/superset/pull/12960?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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org