You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/06 23:04:33 UTC
[GitHub] [superset] ktmud opened a new pull request, #20632: feat(dashboard): make permalink deterministic
ktmud opened a new pull request, #20632:
URL: https://github.com/apache/superset/pull/20632
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Make dashboard permalink API always return the same key if the request payload is the same---i.e., for a given dashboard state, a user should always get a stable permalink when they request one.
This is a required change for #20552 and #19354 as we don't want to generate a new permalink each time a dashboard report is executed.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A. This is a pure backend change. There should be no visible changes to users.
### TESTING INSTRUCTIONS
1. Copy a permalink for dashboard---you can do it in the "..." menu on the dashboard page, or in charts or tabs.
<img width="575" alt="image" src="https://user-images.githubusercontent.com/335541/177657369-ffa725fa-fed4-4328-9877-4edc9a314b8b.png">
3. The link should always be the same no matter how many times you click on the trigger button/link
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] 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
[GitHub] [superset] ktmud merged pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud merged PR #20632:
URL: https://github.com/apache/superset/pull/20632
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r918560508
##########
superset/key_value/utils.py:
##########
@@ -63,3 +64,9 @@ def get_uuid_namespace(seed: str) -> UUID:
md5_obj = md5()
md5_obj.update(seed.encode("utf-8"))
return UUID(md5_obj.hexdigest())
+
+
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+ """Get a deterministic UUID (uuid3) from a salt and a payload."""
+ payload_str = json_dumps_w_dates(payload, sort_keys=True)
Review Comment:
@ktmud not all `KeyValue` implementations will necessarily be JSON serializable. For instance, the `SupersetMetastoreCache` KeyValue objects aren't JSON serialisable. While pickle isn't necessarily perfect in all scenarios, IMO it's the most generally supported serialising/deserializing utility for Python, and as such seems like the best solution for a general purpose key-value store.
At the time this feature was designed there was really no requirement for analysing the contents of the value payload, so this wasn't considered. But if this is needed I don't see any reason why we couldn't make the serializer and deserialiser customizable to make it possible to use another library for encoding/decoding the values and then swapping it out for `json.dumps` and `json.loads` for the permalink implementations (would of course require a migration).
In the case of the proposed function, I'd recommend adding a comment in the doctoring that the payload needs to be JSON serializable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r915353795
##########
superset/key_value/utils.py:
##########
@@ -66,5 +67,11 @@ def get_uuid_namespace(seed: str) -> UUID:
return UUID(md5_obj.hexdigest())
-def get_owner(user: User) -> Optional[int]:
- return user.id if not user.is_anonymous else None
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+ """Get a deterministic UUID (uuid3) from a salt and a payload."""
+ payload_str = json_dumps_w_dates(payload, sort_keys=True)
+ return uuid3(get_uuid_namespace(namespace), payload_str)
+
+
+def get_owner_id(user: Optional[User]) -> Optional[int]:
Review Comment:
Note to self. This should be replaced with [get_user_id](https://github.com/apache/superset/blob/94b3d2f0f0e920e667bf2c2d9d3fbfd9ebcc3ffd/superset/utils/core.py#L1437-L1452) if/when https://github.com/apache/superset/pull/20499 is merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r915463127
##########
superset/dashboards/permalink/commands/create.py:
##########
@@ -40,21 +53,28 @@ def __init__(
self.dashboard_id = dashboard_id
self.state = state
+ @property
+ def value(self) -> DashboardPermalinkValue:
+ return {
+ "dashboardId": self.dashboard_id,
+ "state": self.state,
+ }
+
+ @property
+ def signature(self) -> Tuple[Optional[int], DashboardPermalinkValue]:
Review Comment:
Why is the signature a function of the user?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r916448055
##########
superset/dashboards/permalink/commands/create.py:
##########
@@ -40,21 +53,28 @@ def __init__(
self.dashboard_id = dashboard_id
self.state = state
+ @property
+ def value(self) -> DashboardPermalinkValue:
+ return {
+ "dashboardId": self.dashboard_id,
+ "state": self.state,
+ }
+
+ @property
+ def signature(self) -> Tuple[Optional[int], DashboardPermalinkValue]:
Review Comment:
Just thought it'd be safer to give each user a unique permalink in case we want to anything special about it. (Can't think of a use case just yet) Plus user id is also saved for KeyValue so you can also argue this is for completeness sake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r919409668
##########
superset/key_value/utils.py:
##########
@@ -63,3 +64,9 @@ def get_uuid_namespace(seed: str) -> UUID:
md5_obj = md5()
md5_obj.update(seed.encode("utf-8"))
return UUID(md5_obj.hexdigest())
+
+
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+ """Get a deterministic UUID (uuid3) from a salt and a payload."""
+ payload_str = json_dumps_w_dates(payload, sort_keys=True)
Review Comment:
I still don't think it's a good idea to store language-specific serialized data in databases---even though Superset is a predominant Python app. I can see the case when we need to store other binary data, e.g., image files, cache for zipped exporst, encrypted data, etc, so maybe the `value` column indeed needs to be binary. A customizable serializer sounds like a good idea. Let's revisit when offline analysis becomes a real need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r918216047
##########
superset/key_value/utils.py:
##########
@@ -63,3 +64,9 @@ def get_uuid_namespace(seed: str) -> UUID:
md5_obj = md5()
md5_obj.update(seed.encode("utf-8"))
return UUID(md5_obj.hexdigest())
+
+
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+ """Get a deterministic UUID (uuid3) from a salt and a payload."""
+ payload_str = json_dumps_w_dates(payload, sort_keys=True)
Review Comment:
@villebro @michael-s-molina any particular reason why KeyValue was stored with Python pickle instead of JSON? IMO we should avoid storing pickles in database as they are not portable and could be versioned out if an advanced python object was used. It'd also be impossible to export the database directly for offline analysis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r916448055
##########
superset/dashboards/permalink/commands/create.py:
##########
@@ -40,21 +53,28 @@ def __init__(
self.dashboard_id = dashboard_id
self.state = state
+ @property
+ def value(self) -> DashboardPermalinkValue:
+ return {
+ "dashboardId": self.dashboard_id,
+ "state": self.state,
+ }
+
+ @property
+ def signature(self) -> Tuple[Optional[int], DashboardPermalinkValue]:
Review Comment:
Just thought it'd be safer to give each user a unique permalink in case we want to do anything special about it. (Can't think of a use case just yet) Plus user id is also saved for KeyValue so you can also argue this is for completeness sake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20632: feat(dashboard): make permalink deterministic
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20632:
URL: https://github.com/apache/superset/pull/20632#issuecomment-1176844705
# [Codecov](https://codecov.io/gh/apache/superset/pull/20632?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20632](https://codecov.io/gh/apache/superset/pull/20632?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91ed359) into [master](https://codecov.io/gh/apache/superset/commit/c992ff3be472738a34491d6e8ba8ba10167b2f8c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c992ff3) will **decrease** coverage by `11.93%`.
> The diff coverage is `69.76%`.
```diff
@@ Coverage Diff @@
## master #20632 +/- ##
===========================================
- Coverage 66.82% 54.88% -11.94%
===========================================
Files 1752 1752
Lines 65637 65653 +16
Branches 6940 6940
===========================================
- Hits 43859 36032 -7827
- Misses 20016 27859 +7843
Partials 1762 1762
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.78% <69.76%> (+0.09%)` | :arrow_up: |
| python | `58.14% <69.76%> (-24.75%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.77% <67.44%> (+0.09%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20632?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...uperset/dashboards/filter\_state/commands/create.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc3RhdGUvY29tbWFuZHMvY3JlYXRlLnB5) | `45.83% <50.00%> (-54.17%)` | :arrow_down: |
| [...uperset/dashboards/filter\_state/commands/delete.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc3RhdGUvY29tbWFuZHMvZGVsZXRlLnB5) | `45.83% <50.00%> (-50.01%)` | :arrow_down: |
| [...uperset/dashboards/filter\_state/commands/update.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc3RhdGUvY29tbWFuZHMvdXBkYXRlLnB5) | `40.00% <50.00%> (-60.00%)` | :arrow_down: |
| [superset/explore/form\_data/commands/delete.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZGVsZXRlLnB5) | `47.50% <50.00%> (-45.00%)` | :arrow_down: |
| [superset/explore/form\_data/commands/update.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvdXBkYXRlLnB5) | `40.81% <50.00%> (-53.07%)` | :arrow_down: |
| [superset/key\_value/utils.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL3V0aWxzLnB5) | `82.92% <62.50%> (-11.67%)` | :arrow_down: |
| [superset/key\_value/commands/upsert.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3Vwc2VydC5weQ==) | `52.08% <66.66%> (-37.51%)` | :arrow_down: |
| [superset/dashboards/permalink/commands/create.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9wZXJtYWxpbmsvY29tbWFuZHMvY3JlYXRlLnB5) | `57.57% <72.72%> (-32.08%)` | :arrow_down: |
| [superset/dashboards/permalink/commands/base.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9wZXJtYWxpbmsvY29tbWFuZHMvYmFzZS5weQ==) | `87.50% <85.71%> (-12.50%)` | :arrow_down: |
| [superset/explore/form\_data/commands/create.py](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvY3JlYXRlLnB5) | `42.85% <100.00%> (-52.39%)` | :arrow_down: |
| ... and [289 more](https://codecov.io/gh/apache/superset/pull/20632/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20632?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20632?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c992ff3...91ed359](https://codecov.io/gh/apache/superset/pull/20632?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r915339557
##########
superset/dashboards/permalink/commands/create.py:
##########
@@ -40,21 +53,28 @@ def __init__(
self.dashboard_id = dashboard_id
self.state = state
+ @property
+ def value(self) -> DashboardPermalinkValue:
+ return {
+ "dashboardId": self.dashboard_id,
+ "state": self.state,
+ }
+
+ @property
+ def signature(self) -> Tuple[Optional[int], DashboardPermalinkValue]:
+ return (get_owner_id(self.actor), self.value)
+
def run(self) -> str:
self.validate()
try:
DashboardDAO.get_by_id_or_slug(self.dashboard_id)
- value = {
- "dashboardId": self.dashboard_id,
- "state": self.state,
- }
- key = CreateKeyValueCommand(
+ key = UpsertKeyValueCommand(
+ key=get_deterministic_uuid(self.salt, self.signature),
Review Comment:
Theoretically the uuid3 would never collide with another uuid3, but there is an extremely small chance of colliding with a uuid4. Once we change the Explore permalink to this deterministic id, too, this should not be a concern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r919409668
##########
superset/key_value/utils.py:
##########
@@ -63,3 +64,9 @@ def get_uuid_namespace(seed: str) -> UUID:
md5_obj = md5()
md5_obj.update(seed.encode("utf-8"))
return UUID(md5_obj.hexdigest())
+
+
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+ """Get a deterministic UUID (uuid3) from a salt and a payload."""
+ payload_str = json_dumps_w_dates(payload, sort_keys=True)
Review Comment:
I still don't think it's a good idea to store language-specific serialized data in databases---even though Superset is predominantly a Python app. I can see the case when we need to store other binary data, e.g., image files, cache for zipped exports, encrypted data, etc, so maybe the `value` column indeed needs to be binary. A customizable serializer sounds like a good idea. Let's revisit when offline analysis becomes a real need.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20632: feat(dashboard): make permalinks stable
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20632:
URL: https://github.com/apache/superset/pull/20632#discussion_r918560508
##########
superset/key_value/utils.py:
##########
@@ -63,3 +64,9 @@ def get_uuid_namespace(seed: str) -> UUID:
md5_obj = md5()
md5_obj.update(seed.encode("utf-8"))
return UUID(md5_obj.hexdigest())
+
+
+def get_deterministic_uuid(namespace: str, payload: Any) -> UUID:
+ """Get a deterministic UUID (uuid3) from a salt and a payload."""
+ payload_str = json_dumps_w_dates(payload, sort_keys=True)
Review Comment:
@ktmud not all `KeyValue` implementations will necessarily be JSON serializable. For instance, the `SupersetMetastoreCache` KeyValue objects aren't JSON serialisable. While pickle isn't necessarily perfect in all scenarios, IMO it's the most generally supported serialising/deserializing utility for Python, and as such seems like the best solution for a general purpose key-value store.
At the time this feature was designed there was really no requirement for analysing the contents of the value contents, so this wasn't considered. But if this is needed I don't see any reason why we couldn't make the serializer and deserialiser customizable to make it possible to use another library for encoding/decoding the values and then swapping it out for `json.dumps` and `json.loads` for the permalink implementations (would of course require a migration).
In the case of the proposed function, I'd recommend adding a comment in the doctoring that the payload needs to be JSON serializable.
--
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