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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
