You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2025/05/30 04:08:28 UTC
[PR] fix: Update dataset's last modified date from column/metric update [superset]
Vitor-Avila opened a new pull request, #33626:
URL: https://github.com/apache/superset/pull/33626
### SUMMARY
The `last_modified` value for datasets was not getting updated in case only metric(s) and/or column(s) changed. I suspect the issue started with the introduction of the `@transaction` decorator + the way we handle these updates:
- Before [SIP-99B](https://github.com/apache/superset/issues/25108), we would `commit` after these changes -- [ref](https://github.com/apache/superset/commit/8fb8199a55257ae5121300aa3bd28d14d327bc33#diff-1fabcc7af69ff8ee94b640857ad3552402286c735a5a770b9d14f204774c8f60L269-L322).
- We're currently using `` for the update which does not integrate well with SQLAlchemy's state management (and therefore does not trigger an `after_update`) -- [ref](https://github.com/apache/superset/blob/master/superset/daos/dataset.py#L233-L256).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Before**
https://github.com/user-attachments/assets/9187b29a-bc63-4c6e-9ee7-8db8254cd4bd
**After**
https://github.com/user-attachments/assets/67a82bcc-0f57-4450-a540-1968c1526201
### TESTING INSTRUCTIONS
1. Apply a label to a column, and confirm the `last_modified` value for the dataset gets updated.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "anthonyhungnguyen (via GitHub)" <gi...@apache.org>.
anthonyhungnguyen commented on PR #33626:
URL: https://github.com/apache/superset/pull/33626#issuecomment-2923232423
We're at Geotab and gonna get this fixed soon!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2122184008
##########
superset/daos/dataset.py:
##########
@@ -184,15 +184,21 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ if force_update:
+ attributes["changed_on"] = datetime.now()
Review Comment:
we'll need a follow up for other places tho
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2123503618
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1188,6 +1190,10 @@ def test_update_dataset_create_column_and_metric(self):
metric.pop("type_generic", None)
data["result"]["metrics"].append(new_metric_data)
+
+ # Sleep to ensure that the changed_on is updated
Review Comment:
Optional: You can use `freezegun` to control time and make tests more accurate and efficient.
```
from freezegun import freeze_time
with freeze_time("2021-01-01T00:00:00Z"):
...
```
##########
superset/daos/dataset.py:
##########
@@ -184,15 +184,21 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ if force_update:
+ attributes["changed_on"] = datetime.now(tz=timezone.utc)
Review Comment:
Thanks 🙏🏼
##########
superset/connectors/sqla/models.py:
##########
@@ -2072,8 +2056,6 @@ def load_database(self: SqlaTable) -> None:
sa.event.listen(SqlaTable, "before_update", SqlaTable.before_update)
sa.event.listen(SqlaTable, "after_insert", SqlaTable.after_insert)
sa.event.listen(SqlaTable, "after_delete", SqlaTable.after_delete)
-sa.event.listen(SqlMetric, "after_update", SqlaTable.update_column)
Review Comment:
@Vitor-Avila The previous event-based implementation was triggered for changes from any source. This means that any time `SqlMetric` or `TableColumn` was updated, from any part of the code, the timestamp was updated. Did we check to see if these models are updated outside the DAO?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2123720949
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1188,6 +1190,10 @@ def test_update_dataset_create_column_and_metric(self):
metric.pop("type_generic", None)
data["result"]["metrics"].append(new_metric_data)
+
+ # Sleep to ensure that the changed_on is updated
Review Comment:
@michael-s-molina thank you 🙏 I used that in another test, but here I intentionally want to confirm that the new value for `changed_on` is more recent than the old one, so this should be sufficient. I wanted to avoid mocking it to ensure the logic is properly applied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2124094568
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1188,6 +1190,10 @@ def test_update_dataset_create_column_and_metric(self):
metric.pop("type_generic", None)
data["result"]["metrics"].append(new_metric_data)
+
+ # Sleep to ensure that the changed_on is updated
Review Comment:
ok, this is done now! 🙌 I'll work on a follow up to remove `time.sleep()` from other places I found in the code. I'll probably combine that with a deprecation of `datetime.now()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2116708548
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
@michael-s-molina are you suggesting we always do it?
Technically we could. I opted for this flow because we only need to worry with this in case columns and/or metrics were updated (and no direct dataset attribute was). I don't think there would be a side-effect of always doing, it's just one extra 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #33626:
URL: https://github.com/apache/superset/pull/33626
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2116505690
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
@Vitor-Avila Why not be explicit about this operation and update the `changed_on` field on the dataset? It would avoid the long comment and the cognitive load to understand that `changed_on` will be automatically updated once the record is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2121381833
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
ohhhh, I see! that makes perfect sense. Let me work on that. Thanks @betodealmeida and @michael-s-molina
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2123879916
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1188,6 +1190,10 @@ def test_update_dataset_create_column_and_metric(self):
metric.pop("type_generic", None)
data["result"]["metrics"].append(new_metric_data)
+
+ # Sleep to ensure that the changed_on is updated
Review Comment:
Oh yeah, we should avoid calling `sleep()` on tests. With freezegun you can do:
```python
with freeze_time(initial_datetime) as frozen:
do_something()
frozen.tick(delta=timedelta(seconds=3))
do_something_else()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2116505690
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
@Vitor-Avila Why not be explicit about this operation and update the `changed_on` field on the dataset? It would avoid the whole comments and the cognitive load to understand that `changed_on` will be automatically updated once the record is changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2116894741
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
I think he's suggesting that instead of updating the last modified column as a side effect like you do on line 205, do something that makes the intent clear, something like:
```python
dataset = db.session.query(SqlaTable).get(item.id)
dataset.changed_on = XXX
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2122083702
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
alright, I added `changed_on` to `attributes` so it's processed as any other column.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2123886130
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1188,6 +1190,10 @@ def test_update_dataset_create_column_and_metric(self):
metric.pop("type_generic", None)
data["result"]["metrics"].append(new_metric_data)
+
+ # Sleep to ensure that the changed_on is updated
Review Comment:
oh, TIL -- I can work on that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2120839180
##########
superset/daos/dataset.py:
##########
@@ -184,15 +185,24 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ # columns and metrics are stored in separate tables so if they change we
+ # force an update to the dataset too to ensure the last_updated_timestamp
+ # is updated (it's possible no other dataset attributes are changed).
+ if force_update:
Review Comment:
Exactly @betodealmeida!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #33626:
URL: https://github.com/apache/superset/pull/33626#issuecomment-2935928689
thanks for taking the time to review this @michael-s-molina and @betodealmeida 🙏 I really appreciate the valuable feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2123867098
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1188,6 +1190,10 @@ def test_update_dataset_create_column_and_metric(self):
metric.pop("type_generic", None)
data["result"]["metrics"].append(new_metric_data)
+
+ # Sleep to ensure that the changed_on is updated
Review Comment:
You can do something like:
```
@freeze_time("2023-10-01")
def test_advance_time():
# Check the initial frozen time
assert datetime.now() == datetime(2023, 10, 1)
# Advance time by one day
freeze_time().tick()
# Now it should reflect the new time
assert datetime.now() == datetime(2023, 10, 2)
```
This will make your test run instantly instead of taking 3 seconds to execute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2123857508
##########
superset/connectors/sqla/models.py:
##########
@@ -2072,8 +2056,6 @@ def load_database(self: SqlaTable) -> None:
sa.event.listen(SqlaTable, "before_update", SqlaTable.before_update)
sa.event.listen(SqlaTable, "after_insert", SqlaTable.after_insert)
sa.event.listen(SqlaTable, "after_delete", SqlaTable.after_delete)
-sa.event.listen(SqlMetric, "after_update", SqlaTable.update_column)
Review Comment:
@michael-s-molina the only other place I found that was modifying these is via import flows, which I manually tested and confirmed it is updating the `changed_on` as well, so I believe we're good. Other than that I found some migrations.
If we find other places in the future, happy to do a follow up tho.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2122183072
##########
superset/daos/dataset.py:
##########
@@ -184,15 +184,21 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ if force_update:
+ attributes["changed_on"] = datetime.now()
Review Comment:
it seems that `.utcnow()` is duplicated, so I used `.now(tz=timezone.utc)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: Update dataset's last modified date from column/metric update [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #33626:
URL: https://github.com/apache/superset/pull/33626#discussion_r2122102129
##########
superset/daos/dataset.py:
##########
@@ -184,15 +184,21 @@ def update(
"""
if item and attributes:
+ force_update: bool = False
if "columns" in attributes:
cls.update_columns(
item,
attributes.pop("columns"),
override_columns=bool(attributes.get("override_columns")),
)
+ force_update = True
if "metrics" in attributes:
cls.update_metrics(item, attributes.pop("metrics"))
+ force_update = True
+
+ if force_update:
+ attributes["changed_on"] = datetime.now()
Review Comment:
I think we want `datetime.utcnow` here. I'm in EST:
```python
>>> print(datetime.now(), datetime.utcnow())
2025-06-02 16:29:53.651504 2025-06-02 20:29:53.652092
>>>
```
```suggestion
attributes["changed_on"] = datetime.utcnow()
```
Although I just checked the codebase and it seems like we have some columns where the default is `datetime.now()`. :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org