You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2025/03/05 19:47:09 UTC
[PR] feat: dataset folders (backend) [superset]
betodealmeida opened a new pull request, #32520:
URL: https://github.com/apache/superset/pull/32520
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Tentative implementation of the backend for https://github.com/apache/superset/issues/32351.
I ended up doing the simplest thing that works, based on these requirements:
1. We want to reuse existing dataset APIs (there will be no new APIs to manage folders).
2. Order of folders is important, as well as the order of elements within folders.
3. Support for nested folders should be present from the start (new requirement).
I started the implementation with a `Folder` model, and mapped relationships:
- `Folder n:1 SqlaTable`
- `SqlMetric n:1 Folder`
- `TableColumn n:1 Folder`
One problem with this approach is that order is important, so I had to keep track of the order of each folder and each element inside a folder. This required additional columns, and complex bookkeeping when a dataset was updated — did the positions change? Were metrics removed, columns added, folders renamed? Moving the last element in a folder to the first position, for example, would require updating the position of all elements inside the folder.
Additionally, representing nested folders would require additional relationships to be tracked.
I ditched that approach, and instead opted to serialize the folder structure to a new JSON column in the `SqlaTable` model called `folders`. When doing a `GET` request to `/api/v1/dataset/` or `/api/v1/explore/` the response now includes UUIDs for metrics and columns, and has the new attribute `folders`:
```python
{
...
"metrics": [
{
"metric_name": "count",
"uuid": "uuid2",
...
},
...
],
"columns": [
{
"column_name": "country",
"uuid": "uuid5",
...
},
{
"column_name": "column-not-in-any-folder",
"uuid": "uuid6",
...
},
...
],
"folders": [
{
"type": "folder",
"uuid": "uuid1",
"name": "My metrics",
"children": [
{
"type": "metric",
"uuid": "uuid2",
"name": "count",
},
],
},
{
"type": "folder",
"uuid": "uuid3",
"name": "My columns",
"children": [
{
"type": "folder",
"uuid": "uuid4",
"name": "Dimensions",
"children": [
{
"type": "column",
"uuid": "uuid5",
"name": "country",
},
],
},
],
},
]
}
```
Note that for metrics and columns `type` and `name` are redundant (since they're still present in the resposne payload under `metrics` and `columns`, respectively). But to make the API clearer I uncluded them in the response, even if technically only the UUID is needed.
With this solution the frontend can easily build the UI from this response. Note that the payload only includes custom folders, and the `metrics` and `columns` attribute in the response are unmodified (meaning a metric that is present in a folder will still show up under `metrics`). It's up to the frontend to build the existing "Columns" and "Metrics" sections by removing any elements that are present in custom folders. This way we can build the feature progressively by first enhancing the API, and later adding the custom UI.
To organize the metrics and columns into folders, as well as create new folders, the user must edit the dataset (dataset creation doesn't show metrics nor columns). After creating folders and adding metrics/columns to them the user can save the dataset. The `PUT` request will then send a payload that can also be enhanced with the `folders` attribute.
There is no model for folders, since it offered little to no value. Instead, the client simply declare the folder name, UUID, and an optional description, as well as the tree structure. The UUID is used for external bookkeeping, for example, for systems where semantics are defined outside of Superset and periodically synced via the API.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Added tests.
### 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] feat: dataset folders (backend) [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1996272942
##########
superset/datasets/schemas.py:
##########
@@ -88,6 +88,18 @@ class DatasetMetricsPutSchema(Schema):
uuid = fields.UUID(allow_none=True)
+class FolderSchema(Schema):
+ uuid = fields.UUID()
Review Comment:
@betodealmeida does this UUID need to be generated on the client if it's required here?
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2795678744
> NOTE: ran into something a little funky where both this PR and #32680 were running the same db migration with different names. Removed the one here as a duplicate of the other.
I had spit the migration into a separate PR by @sad's request, and this beach was based on the migration branch IIRC.
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2737649152
I think we should decouple the issues around handling datasets with lots of columns from this PR, though it seems it needs attention.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2734083347
> Are you planning to store thousands of columns/metrics inside folders?
Once the feature is there, we'll probably organize our thousands of metrics/columns in folders by domain. In other words, all metrics/columns will probably reside in one folder.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2755095524
> My concern is that by using a simple API, this feature will not work or be highly inefficient for companies with big datasets which is our case. How separating the API will work in practice? Are you going to launch the feature just to a subset of companies? If at the end we'll need to support big datasets, why pay the cost twice?
Well, what I'm advocating is that we solve the problem we have today — where updating a dataset with tens of thousands of columsn takes too long — in a separate PR. The API is already slow, so I don't see the point in optimizing the folder updates when we would still be sending all the columns and all the metrics when we do an update.
Here's my proposal: it seems like we need a new `PATCH` API for dataset updates. We should add the endpoint, and use a standardized DSL for deltas, like [JSON Patch](https://en.wikipedia.org/wiki/JSON_Patch), which has Python and npm implementations. It would be used for updateing columns, metrics, and folders., and the frontend would be updated to use it, since it's important to be responsive in an interactive environment like the browser.
But we should keep the original `PUT` API for updates that exists today (and with the changes in this PR) because (1) it preserves backwards compatibility, and because (2) it's useful for non-interactive clients where response time is irrelevant. This way, someone who wants to sync datasets from dbt or Cube could use the much simpler `PUT` API, where the whole payload can be sent at once.
So what I'm asking is: let's merge this PR, and I'll work on the `PATCH` endpoint that will solve all the problems.
--
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] feat: dataset folders (backend) [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1993789505
##########
superset/commands/dataset/update.py:
##########
@@ -189,3 +204,60 @@ def _get_duplicates(data: list[dict[str, Any]], key: str) -> list[str]:
if count > 1
]
return duplicates
+
+
+def validate_folders( # noqa: C901
+ folders: list[FolderSchema],
+ metrics: list[SqlMetric],
+ columns: list[TableColumn],
+) -> None:
+ """
+ Additional folder validation.
+
+ The marshmallow schema will validate the folder structure, but we still need to
+ check that UUIDs are valid, names are unique and not reserved, and that there are
+ no cycles.
+ """
+ if not is_feature_enabled("DATASET_FOLDERS"):
+ raise ValidationError("Dataset folders are not enabled")
+
+ existing = {
+ "metric": {metric.uuid: metric.metric_name for metric in metrics},
+ "column": {column.uuid: column.column_name for column in columns},
+ }
+
+ queue: list[tuple[FolderSchema, list[str]]] = [(folder, []) for folder in folders]
Review Comment:
Nice!
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2734694096
I agree with making it scalable and targeting, and I'm happy to make the API response/request payloads more compact.
But in regards to pagination, I don't think the current dataset API supports pagination for metrics and columns? As far as I can tell even if you have 10k metrics they are all included in the response, and the frontend does the pagination/search, is that correct @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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2795023236
NOTE: ran into something a little funky where both this PR and #32680 were running the same db migration with different names. Removed the one here as a duplicate of the other.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754560510
> I've made the API more compact, requiring only the UUIDs for metrics and columns.
Thanks for the improvements @betodealmeida.
> Do you think that would be enough for your use case?
I think the best way to test the feature is to generate a dataset with thousands of columns and metrics and simulate the interactions.
The script below will create a CSV file containing 500 columns and 500 metrics that can be imported into Superset.
```python
import csv
# Define the number of columns, metrics, and rows
num_columns = 500
num_metrics = 500
num_rows = 10
# Create column headers
column_headers = [f"c_{i}" for i in range(1, num_columns + 1)] + [
f"m_{i}" for i in range(1, num_metrics + 1)
]
# Generate sample data
data = [
[
(
f"{row}_{col}"
if col < num_columns
else row * num_metrics + (col - num_columns)
)
for col in range(num_columns + num_metrics)
]
for row in range(num_rows)
]
# Write to CSV file
with open("large_dataset.csv", "w", newline="") as csvfile:
writer = csv.writer(csvfile)
writer.writerow(column_headers)
writer.writerows(data)
print("CSV file 'large_dataset.csv' created successfully.")
```
It's still only 1000 columns which is way less than our 30k but it's a start. If you have another way of creating a dataset in Superset with more columns it would be even better.
Once ready, I'll test the feature using our real data but the test dataset can help making sure the payload and algorithms to edit the folder can handle a lot of data efficiently.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2704364726
/korbit-review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1983721018
##########
superset/datasets/schemas.py:
##########
@@ -88,6 +88,18 @@ class DatasetMetricsPutSchema(Schema):
uuid = fields.UUID(allow_none=True)
+class FolderSchema(Schema):
+ uuid = fields.UUID()
+ type = fields.String(
+ required=False,
+ validate=OneOf(["metric", "column", "folder"]),
+ )
+ name = fields.String(required=True, validate=Length(1, 250))
+ description = fields.String(allow_none=True, validate=Length(0, 1000))
+ # folder can contain metrics, columns, and subfolders:
+ children = fields.List(fields.Nested(lambda: FolderSchema()), allow_none=True)
Review Comment:
We can add this to `validate_folders`
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2794974427
Fixed the issue in master here too
--
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] feat: dataset folders (backend) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1996022845
##########
superset/migrations/versions/2025-03-03_20-52_94e7a3499973_add_folder_table.py:
##########
@@ -0,0 +1,42 @@
+# 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 folder table
+
+Revision ID: 94e7a3499973
+Revises: 74ad1125881c
+Create Date: 2025-03-03 20:52:24.585143
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.types import JSON
+
+# revision identifiers, used by Alembic.
+revision = "94e7a3499973"
+down_revision = "74ad1125881c"
+
+
+def upgrade():
+ op.add_column(
+ "tables",
+ sa.Column("folders", JSON, nullable=True),
Review Comment:
we now have util functions for adding and dropping columns that we'll want to use.
--
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] feat: dataset folders (backend) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2701910933
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754646954
> But then how would we indicate that a metric or column was removed from a folder?
The delta contains this type of information. It contains only the folders that had any change and the type of change made. Something like:
```
"changed_folders": [
"uuid": "uuid1",
"added": ["colum_a", "column_b"],
"removed": ["colum_c"],
]
```
This is just an example, we can make the payload even more compact.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2755126535
> So what I'm asking is: let's merge this PR, and I'll work on the PATCH endpoint that will solve all the problems.
@betodealmeida I'm ok with this approach if by the time we launch the folders feature in an official release (5.1?) it uses the PATCH endpoint 👍🏼
I appreciate your work on the feature and I think it will bring a lot of value. I know my comments make your job harder but I'm just trying to make sure the principles that @villebro mentioned above are followed and that my org can use it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] feat: dataset folders (backend) [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1996275226
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1563,6 +1563,92 @@ def test_update_dataset_sqlalchemy_error(self, mock_dao_update):
db.session.delete(dataset)
db.session.commit()
+ @with_feature_flags(DATASET_FOLDERS=True)
+ def test_update_dataset_add_folders(self):
+ """
+ Dataset API: Test adding folders to dataset
+ """
+ self.login(username="admin")
+
+ dataset = self.insert_default_dataset()
+ dataset_data = {
+ "folders": [
+ {
+ "type": "folder",
+ "uuid": "b49ac3dd-c79b-42a4-9082-39ee74f3b369",
+ "name": "My metrics",
+ "children": [
+ {
+ "type": "metric",
+ "uuid": dataset.metrics[0].uuid,
+ "name": dataset.metrics[0].metric_name,
+ },
+ ],
+ },
+ {
+ "type": "folder",
+ "uuid": "f5db85fa-75d6-45e5-bdce-c6194db80642",
+ "name": "My columns",
+ "children": [
+ {
+ "type": "folder",
+ "uuid": "b5330233-e323-4157-b767-98b16f00ca93",
+ "name": "Dimensions",
+ "children": [
+ {
+ "type": "column",
+ "uuid": dataset.columns[1].uuid,
+ "name": dataset.columns[1].column_name,
Review Comment:
Not a big deal, but when using the api, it seems unnecessary to need the uuid and the name? We should be able to identify it by just the type and uuid?
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1996298949
##########
superset/datasets/schemas.py:
##########
@@ -88,6 +88,18 @@ class DatasetMetricsPutSchema(Schema):
uuid = fields.UUID(allow_none=True)
+class FolderSchema(Schema):
+ uuid = fields.UUID()
Review Comment:
Right, the client needs to generate a UUID when creating folders. They are only used by tools that want to keep track of external folders and Superset folders.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754579774
> folder1 = ["a", "b", "c"]
> folder1 = ["a", "b", "c", "f"] <-- added "f"
For example, if we added "f" to folder1, we shouldn't send all other columns in the request. If the folder contains 5k columns, this means that we're sending 4,999 unnecessary columns in the payload. We should probably only send the deltas.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2733580913
Thanks for the PR @betodealmeida. Some thoughts:
> Note that for metrics and columns type and name are redundant (since they're still present in the response payload under metrics and columns, respectively). But to make the API clearer I uncluded them in the response, even if technically only the UUID is needed.
This might be problematic for us given that we have a dataset with 30k metrics and 7k columns and this decision would significantly increase the payload size.
I think we'll need a very compact API response and a very efficient algorithm when changing the order of things considering the volume of metrics/dimensions. cc @justinpark @villebro
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754653209
It's the same principle of Git when only the changes are sent.
--
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] feat: dataset folders (backend) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1983715531
##########
superset/datasets/schemas.py:
##########
@@ -88,6 +88,18 @@ class DatasetMetricsPutSchema(Schema):
uuid = fields.UUID(allow_none=True)
+class FolderSchema(Schema):
+ uuid = fields.UUID()
+ type = fields.String(
+ required=False,
+ validate=OneOf(["metric", "column", "folder"]),
+ )
+ name = fields.String(required=True, validate=Length(1, 250))
+ description = fields.String(allow_none=True, validate=Length(0, 1000))
+ # folder can contain metrics, columns, and subfolders:
+ children = fields.List(fields.Nested(lambda: FolderSchema()), allow_none=True)
Review Comment:
### Unbounded Recursive Nesting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The recursive FolderSchema definition allows unlimited nesting depth, which could lead to stack overflow or excessive memory usage.
###### Why this matters
Without a maximum depth limit, deeply nested folder structures could cause performance issues or crashes.
###### Suggested change ∙ *Feature Preview*
Add a depth parameter to limit nesting:
```python
class FolderSchema(Schema):
def __init__(self, max_depth=5, current_depth=0, *args, **kwargs):
super().__init__(*args, **kwargs)
self.max_depth = max_depth
self.current_depth = current_depth
children = fields.List(
fields.Nested(
lambda: FolderSchema(max_depth=self.max_depth, current_depth=self.current_depth + 1)
if self.current_depth < self.max_depth
else None
),
allow_none=True
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9e9a275f-9f2f-48ba-bb56-7e5a54a42ee4?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e3ccfe92-95d8-4475-8874-69cd84b53e47 -->
##########
superset/migrations/versions/2025-03-03_20-52_94e7a3499973_add_folder_table.py:
##########
@@ -0,0 +1,42 @@
+# 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 folder table
+
+Revision ID: 94e7a3499973
+Revises: 74ad1125881c
+Create Date: 2025-03-03 20:52:24.585143
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.types import JSON
+
+# revision identifiers, used by Alembic.
+revision = "94e7a3499973"
+down_revision = "74ad1125881c"
+
+
+def upgrade():
+ op.add_column(
+ "tables",
+ sa.Column("folders", JSON, nullable=True),
+ )
Review Comment:
### Non-normalized JSON storage design <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The column name 'folders' suggests storing multiple folders in a JSON field, which violates database normalization principles and makes querying/indexing inefficient.
###### Why this matters
Storing multiple values in a JSON column creates difficulties in data integrity, search performance, and relationship management. It's harder to enforce referential integrity and perform efficient queries on nested JSON data.
###### Suggested change ∙ *Feature Preview*
Create a separate `folders` table with proper foreign key relationships to the `tables` table:
```python
def upgrade():
op.create_table(
'folders',
sa.Column('id', sa.Integer(), primary_key=True),
sa.Column('table_id', sa.Integer(), sa.ForeignKey('tables.id')),
sa.Column('name', sa.String(length=250))
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e9e9d9ed-30ac-4f99-ba3b-75e42c3ea462?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:78d07164-54e2-412c-b8e0-339e1cfe2220 -->
--
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] feat: dataset folders (backend) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1983721458
##########
superset/datasets/schemas.py:
##########
@@ -88,6 +88,18 @@ class DatasetMetricsPutSchema(Schema):
uuid = fields.UUID(allow_none=True)
+class FolderSchema(Schema):
+ uuid = fields.UUID()
+ type = fields.String(
+ required=False,
+ validate=OneOf(["metric", "column", "folder"]),
+ )
+ name = fields.String(required=True, validate=Length(1, 250))
+ description = fields.String(allow_none=True, validate=Length(0, 1000))
+ # folder can contain metrics, columns, and subfolders:
+ children = fields.List(fields.Nested(lambda: FolderSchema()), allow_none=True)
Review Comment:
Thanks for your feedback! We will mute issues related to `Unbounded Recursive Nesting` in the future. You can find all of your mutes and policy changes in your admin dashboard.
--
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] feat: dataset folders (backend) [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #32520:
URL: https://github.com/apache/superset/pull/32520
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2794701036
Fixed gnarly merge conflict while carrying master through https://github.com/apache/superset/pull/32534 and https://github.com/apache/superset/pull/32520 🤞 🤞
--
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] feat: dataset folders (backend) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1993571403
##########
superset/datasets/schemas.py:
##########
@@ -88,6 +88,18 @@ class DatasetMetricsPutSchema(Schema):
uuid = fields.UUID(allow_none=True)
+class FolderSchema(Schema):
+ uuid = fields.UUID()
+ type = fields.String(
+ required=False,
+ validate=OneOf(["metric", "column", "folder"]),
+ )
Review Comment:
### Non-required type field compromises folder functionality <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'type' field in FolderSchema is marked as not required but should be required as it's essential for folder functionality
###### Why this matters
Without a required type field, the system won't be able to determine how to handle the folder's contents, potentially causing runtime errors
###### Suggested change ∙ *Feature Preview*
Make the type field required:
```python
type = fields.String(
required=True,
validate=OneOf(["metric", "column", "folder"]),
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/becc07ce-355d-4113-8179-e53f250942c7?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e23a1ff1-a533-431c-aea4-67ff305ead57 -->
##########
superset/migrations/versions/2025-03-03_20-52_94e7a3499973_add_folder_table.py:
##########
@@ -0,0 +1,42 @@
+# 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 folder table
+
+Revision ID: 94e7a3499973
+Revises: 74ad1125881c
+Create Date: 2025-03-03 20:52:24.585143
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.types import JSON
+
+# revision identifiers, used by Alembic.
+revision = "94e7a3499973"
+down_revision = "74ad1125881c"
+
+
+def upgrade():
+ op.add_column(
+ "tables",
+ sa.Column("folders", JSON, nullable=True),
Review Comment:
### Ambiguous column name for JSON structure <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The column name 'folders' is ambiguous and doesn't clearly indicate its purpose or structure in the database schema.
###### Why this matters
Without a more descriptive name, future developers will need to investigate other parts of the codebase to understand what type of folders are stored and how they are structured in the JSON field.
###### Suggested change ∙ *Feature Preview*
Rename the column to be more specific, such as:
```python
sa.Column("dataset_folder_config", JSON, nullable=True)
```
or
```python
sa.Column("column_folder_hierarchy", JSON, nullable=True)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e8b2c988-6b0f-4669-abf9-d31239aa62c9?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1ca87f05-4223-4155-8aed-766811c8b526 -->
##########
superset/commands/dataset/update.py:
##########
@@ -127,16 +128,30 @@ def validate(self) -> None:
except ValidationError as ex:
exceptions.append(ex)
- # Validate columns
+ self._validate_semantics(exceptions)
+
+ if exceptions:
+ raise DatasetInvalidError(exceptions=exceptions)
+
+ def _validate_semantics(self, exceptions: list[ValidationError]) -> None:
+ # we know we have a valid model
+ self._model = cast(SqlaTable, self._model)
+
if columns := self._properties.get("columns"):
self._validate_columns(columns, exceptions)
- # Validate metrics
if metrics := self._properties.get("metrics"):
self._validate_metrics(metrics, exceptions)
- if exceptions:
- raise DatasetInvalidError(exceptions=exceptions)
+ if folders := self._properties.get("folders"):
+ try:
+ validate_folders(folders, self._model.metrics, self._model.columns)
+ except ValidationError as ex:
+ exceptions.append(ex)
Review Comment:
### Lost validation error context <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error handling silently collects ValidationError exceptions without preserving or logging any context about where or why the validation failed.
###### Why this matters
This makes debugging validation failures more difficult since the original error context and stack trace are lost.
###### Suggested change ∙ *Feature Preview*
Modify the error handling to preserve context:
```python
try:
validate_folders(folders, self._model.metrics, self._model.columns)
except ValidationError as ex:
logger.warning(
"Folder validation failed",
exc_info=True,
extra={"folders": folders},
)
exceptions.append(ex)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c788b1bc-189f-41d1-9786-96f49a0a6b9b?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0aa8493d-37e7-4a4d-9edf-f567437c4000 -->
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r2040096359
##########
superset-frontend/package-lock.json:
##########
@@ -50863,7 +50863,7 @@
"version": "0.20.3",
"license": "Apache-2.0",
"dependencies": {
- "@types/react-redux": "^7.1.10",
+ "@types/react-redux": "^7.1.34",
Review Comment:
a [desired AFAIK] artifact from `docker-compose up` running `npm i`, I think we should merge this along with any PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] feat: dataset folders (backend) [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2737666328
> I think we should decouple the issues around handling datasets with lots of columns from this PR, though it seems it needs attention.
Agreed, I don't think changing the current dataset API for related columns and metrics is in scope for this PR. But as we know this is would probably cause a similar problem in folders, I feel like we should try to design this feature to not have the same scalability issue.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2734061288
@michael-s-molina to clarify, only metrics and columns that belong to a folder are sent in the `folders` attribute. The new response payload has `metrics` and `columns` as before (same size and content, for backwards compatibility), and `folders` has only the new information (folder names, UUID, and a short description of metrics/columns they contain — UUID, type, name.
Are you planning to store thousands of columns/metrics inside folders?
We can always have `folders` be just the UUIDs and folder names, for compactness. The only reason I added was to make the response payload more readable, but that's a dev UX, and definitely not needed.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2736263055
> As far as I can tell even if you have 10k metrics they are all included in the response, and the frontend does the pagination/search, is that correct @michael-s-molina?
Yes, the metrics are all included in the response and there's no pagination made by the frontend. It just renders everything without pagination and virtualization. Apart from that, another huge problem of that module is that it sends ALL columns/metrics to server side when saving instead of just sending the delta which is really inefficient. To put this is perspective, it takes more than 30 seconds to load that screen for our biggest dataset and more than a minute to make changes.
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2733764490
> given that we have a dataset with 30k metrics and 7k columns
Seems important to **not** store the "unclassified" items in the "folders" serialized payload. Not sure how it's behaving currently as I haven't deeply reviewed the PR, but seems like a good idea to only serialize/store items that have been actively put inside a folder.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2734110482
> > Are you planning to store thousands of columns/metrics inside folders?
>
> Once the feature is there, we'll probably organize our thousands of metrics/columns in folders by domain. In other words, all metrics/columns will probably reside in one folder.
OK, let me modify it to have only UUIDs.
--
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] feat: dataset folders (backend) [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2734215385
On Michael's point about having thousands of metrics and columns, I think it touches on an important design requirement to always aim for being able to handle considerable scale, even if it's not obvious at first. Some examples:
- We take full use of indexes in the data model, and typically have all logical models in dedicated tables with link tables for efficient RDBMS handling.
- In migrations we've established patterns for paginating updates.
- For listings we paginate whenever possible (both on frontend and API endpoints).
- For the FE bundle, we minimize the bundle size maximally, and split everything into chunks whenever possible.
- We always defer chart rendering to only the visible charts.
- We've invested extensively into reducing FE overrendering.
- For rendering chart queries, we've replaced `sqlparse` with `sqlglot` due to wasteful handling of the query AST.
- etc
In isolation, all of these would likely have seemed like overkill initially, but for each, their absence proved highly damaging as things grew. The point I'm trying to make is I think we need to always consider how something works when taken to the limit, even if initial use cases don't require it.
As Airbnb's requirement would likely be solved using the already battle tested pattern of having dedicated entity tables with efficient indexing + paginated APIs, I feel we should design this from the start with high scalability in mind, so as to make sure we don't have to do significant refactoring to the feature later on.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2755096716
@betodealmeida I think the important part of the discussion is that we're adding a new feature to Superset and that feature needs to work with our real data which includes datasets with thousands of columns/metrics. Like @villebro said above:
> it touches on an important design requirement to always aim for being able to handle considerable scale, even if it's not obvious at first.
> The point I'm trying to make is I think we need to always consider how something works when taken to the limit, even if initial use cases don't require it.
I suggested the delta approach as a possible solution but you might come up with another one that also works and that would be great too.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754332613
@michael-s-molina I've made the API more compact, requiring only the UUIDs for metrics and columns.
I can also change the API so that the request payload only needs to contain changes. For example, if we have these folders:
```
folder1 = ["a", "b", "c"]
folder2 = ["d"]
folder3 = ["e", "f"]
folder4 = ["g"]
```
Let's say the user:
1. Moves "f" from "folder3" to "folder1"
2. Deletes "folder2"
Then the payload would be (in pseudocode):
```
folder1 = ["a", "b", "c", "f"]
folder3 = ["e"]
folder4 = []
```
Instead of sending the whole new state. Do you think that would be enough for your use case?
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2795692622
I think I'm mostly through. Great code and TIL about unpacking with `*[]`. So new that both AI and I were a bit confused about it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2787389390
Resolved the merge conflict to push this forward
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2737684760
> I think we should decouple the issues around handling datasets with lots of columns from this PR, though it seems it needs attention.
@mistercrunch I agree that the previous implementation of dataset editor can be tackled in a separate PR. What I think should be tackled in this PR is to make sure the new feature supports thousands of columns/metrics. In other words, the algorithms and strategies used need to consider this requirement. What we can't do is to make the problem even worse.
--
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] feat: dataset folders (backend) [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2702097861
Pointing out something related here, is that the tree component is likely going to be composed of react nodes (not just the description). Today we have [from memory, components names may not match] MetricLabel, ColumnLabel, CalculatedColumnLabel, and these require props beyond what's in folders (description to make a InfoBubbleTooltip, sql_expression, data type, ...). Assuming we want for a rich tree with the full labels (at least on the left panel in explore), this means we'll still have to lookup the related objects from the API.
Thinking about future frontend development, we'll need some sort of `assembleTreeDataForComponent(apiPayload)` method, and that it'll have to build rich react nodes. One the other side, we'll need something that takes the `AntdTree.treeData` and prepares the POST payload required by the API, this one should be fairly simple I think.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754697655
Thanks, I know how git works.
What I'm asking is, we now need to define a DSL for the payload, to represent deltas. This makes the flow much more complicated for 95-99% of the cases, so I'd much rather have the simple API for the common use cases, and later implement a delta API (maybe using PATCH) for cases like this.
The new API could then be also used for columns and metrics.
--
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] feat: dataset folders (backend) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754966194
> I'd much rather have the simple API for the common use cases, and later implement a delta API (maybe using PATCH) for cases like this.
My concern is that by using a simple API, this feature will not work or be highly inefficient for companies with big datasets which is our case. How separating the API will work in practice? Are you going to launch the feature just to a subset of companies? If at the end we'll need to support big datasets, why paying the cost twice?
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #32520:
URL: https://github.com/apache/superset/pull/32520#issuecomment-2754619097
> > folder1 = ["a", "b", "c"]
> > folder1 = ["a", "b", "c", "f"] <-- added "f"
>
> For example, if we added "f" to folder1, we shouldn't send all other columns in the request. If the folder contains 5k columns, this means that we're sending 4,999 unnecessary columns in the payload. We should probably only send the deltas.
>
>
>
But then how would we indicate that a metric or column was removed from a folder?
--
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] feat: dataset folders (backend) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1996023206
##########
superset/migrations/versions/2025-03-03_20-52_94e7a3499973_add_folder_table.py:
##########
@@ -0,0 +1,42 @@
+# 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 folder table
+
+Revision ID: 94e7a3499973
+Revises: 74ad1125881c
+Create Date: 2025-03-03 20:52:24.585143
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.types import JSON
+
+# revision identifiers, used by Alembic.
+revision = "94e7a3499973"
+down_revision = "74ad1125881c"
+
+
+def upgrade():
+ op.add_column(
+ "tables",
+ sa.Column("folders", JSON, nullable=True),
Review Comment:
Thank you for implementing this feature. I agree with your approach of creating a serialized folder structure in a new JSON column. However, I would suggest renaming column 'folders' to 'folder_hierarchy' or 'folder_structure' for better clarity and context. This would give a clear hint about the structure without being too verbose. Additionally, could you please utilize the existing util functions for adding and dropping columns, that would help in maintaining consistent coding patterns across the repo.
--
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] feat: dataset folders (backend) [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #32520:
URL: https://github.com/apache/superset/pull/32520#discussion_r1996303034
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1563,6 +1563,92 @@ def test_update_dataset_sqlalchemy_error(self, mock_dao_update):
db.session.delete(dataset)
db.session.commit()
+ @with_feature_flags(DATASET_FOLDERS=True)
+ def test_update_dataset_add_folders(self):
+ """
+ Dataset API: Test adding folders to dataset
+ """
+ self.login(username="admin")
+
+ dataset = self.insert_default_dataset()
+ dataset_data = {
+ "folders": [
+ {
+ "type": "folder",
+ "uuid": "b49ac3dd-c79b-42a4-9082-39ee74f3b369",
+ "name": "My metrics",
+ "children": [
+ {
+ "type": "metric",
+ "uuid": dataset.metrics[0].uuid,
+ "name": dataset.metrics[0].metric_name,
+ },
+ ],
+ },
+ {
+ "type": "folder",
+ "uuid": "f5db85fa-75d6-45e5-bdce-c6194db80642",
+ "name": "My columns",
+ "children": [
+ {
+ "type": "folder",
+ "uuid": "b5330233-e323-4157-b767-98b16f00ca93",
+ "name": "Dimensions",
+ "children": [
+ {
+ "type": "column",
+ "uuid": dataset.columns[1].uuid,
+ "name": dataset.columns[1].column_name,
Review Comment:
Right, I considered making them optional.
I included them in the `GET` API because it makes it easier to understand the API response, even thought they are redundant. But I think we could make them optional in the `PUT`, and infer types/names from the existing metrics/columns.
--
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