codeant-ai-for-open-source[bot] commented on code in PR #43757:
URL: https://github.com/apache/superset/pull/43757#discussion_r4054152721
##########
superset/commands/dataset/update.py:
##########
@@ -291,6 +295,8 @@ def _validate_semantics(self, exceptions:
list[ValidationError]) -> None:
if predicate := self._properties.get("fetch_values_predicate"):
self._validate_fetch_values_predicate(predicate, exceptions)
+ self._validate_partition_mapping(exceptions)
Review Comment:
**Suggestion:** `_validate_partition_mapping` runs even when
`PARTITION_FILTER_MAPPING` is disabled, so disabled deployments can still
reject dataset updates because of mapping validation.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=14262ca720384a23b126361b3fb54951&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=14262ca720384a23b126361b3fb54951&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 298:298
**Comment:**
*Logic Error: `_validate_partition_mapping` runs even when
`PARTITION_FILTER_MAPPING` is disabled, so disabled deployments can still
reject dataset updates because of mapping validation.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=c81fd09aba3d18e8c3d00aaf92ee22a3e78a11f1b5f5cdea5dd7fe58f7a4671a&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=c81fd09aba3d18e8c3d00aaf92ee22a3e78a11f1b5f5cdea5dd7fe58f7a4671a&reaction=dislike'>๐</a>
##########
superset/commands/dataset/update.py:
##########
@@ -399,6 +405,103 @@ def _validate_expressions(
)
)
+ def _validate_partition_mapping(self, exceptions: list[ValidationError])
-> None:
+ """
+ Validate the dataset's partition filter mapping.
+
+ Only the blocking (Tier 1) issues become validation errors. Tier 2
+ issues -- an unparseable transform, a transform missing `:value` --
+ deliberately let the save through and leave the mapping inactive, per
+ the PRD, so a half-written transform doesn't cost the owner the rest of
+ their edits. They are surfaced by the editor, not by rejecting the PUT.
+
+ The transform is authored by a dataset owner, the same principal and
+ trust level as a calculated-column expression, so it also goes through
+ `validate_stored_expression` -- the parser gate that already governs
+ stored expressions.
+ """
+ self._model = cast(SqlaTable, self._model)
+
+ columns = self._properties.get("columns")
+ column_names = (
+ {column["column_name"] for column in columns}
+ if columns is not None
+ else {column.column_name for column in self._model.columns}
+ )
+
+ partition_column = self._properties.get(
+ "partition_column", self._model.partition_column
+ )
+ partition_mapped_column = self._properties.get(
+ "partition_mapped_column", self._model.partition_mapped_column
+ )
+ main_dttm_col = self._properties.get("main_dttm_col",
self._model.main_dttm_col)
+ if not partition_column:
+ return
+
+ database = self._properties.get("database") or self._model.database
+ catalog = self._properties.get("catalog", self._model.catalog)
+ schema = self._properties.get("schema", self._model.schema)
+
+ effective_mapped_column = partition_mapped_column or main_dttm_col
+ transform = self._effective_transform(columns, effective_mapped_column)
+
+ for issue in validate_partition_mapping(
+ column_names=column_names,
+ partition_column=partition_column,
+ partition_mapped_column=partition_mapped_column,
+ main_dttm_col=main_dttm_col,
+ transform=transform,
+ engine=database.backend,
+ ):
+ if issue.blocking:
+ exceptions.append(
+ ValidationError(str(issue.message), field_name=issue.field)
+ )
+
+ if transform:
+ try:
+ validate_stored_expression(
+ database, catalog, schema, parse_skeleton(transform)
+ )
Review Comment:
**Suggestion:** An unparseable transform without Jinja still reaches
`validate_stored_expression`, which raises a validation error and rejects the
save despite the documented non-blocking behavior.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=89c34c9ae26c4cb3b389b7ce19eda10f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=89c34c9ae26c4cb3b389b7ce19eda10f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 462:466
**Comment:**
*Api Mismatch: An unparseable transform without Jinja still reaches
`validate_stored_expression`, which raises a validation error and rejects the
save despite the documented non-blocking behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=86071a26b75c58d68873c4b55941f5b62a80264baea385225b2b15d7dfc82f77&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=86071a26b75c58d68873c4b55941f5b62a80264baea385225b2b15d7dfc82f77&reaction=dislike'>๐</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1894,8 +1925,44 @@ def data(self) -> ExplorableData:
data_["extra"] = self.extra
data_["always_filter_main_dttm"] = self.always_filter_main_dttm
data_["normalize_columns"] = self.normalize_columns
+ data_["partition_column"] = self.partition_column
+ data_["partition_mapped_column"] = self.partition_mapped_column
+ data_["partition_filter_mapping"] =
self.partition_filter_mapping_summary
return data_
+ @property
+ def partition_filter_mapping_summary(self) -> dict[str, Any] | None:
+ """
+ Self-contained summary of the mapping for the Explore indicator.
+
+ Deliberately not a lookup into `columns`: `data_for_slices` prunes
+ columns no chart references, and the partition column is typically
+ referenced by none of them, so anything reading it out of
+ `datasource.columns` would work in Explore and break on dashboards.
+
+ `active` is derived from cheap signals only. This property is
serialized
+ on every chart and dashboard load, so parsing the transform here would
+ put a per-request cost on a hot path for a value that only changes on
+ save.
+ """
+ if not self.partition_column:
+ return None
+
+ columns_by_name = {column.column_name: column for column in
self.columns}
+ mapped_column_name = self.partition_mapped_column or self.main_dttm_col
+ mapped_column = columns_by_name.get(mapped_column_name or "")
+ active = bool(
+ self.partition_column in columns_by_name
+ and mapped_column is not None
+ and mapped_column_name != self.partition_column
+ and (mapped_column.partition_value_transform or "").strip()
+ )
Review Comment:
**Suggestion:** `active` becomes true for any nonblank transform, including
missing `:value`, invalid SQL, Jinja, or nondeterministic functions that
validation leaves inactive.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Incorrect
condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=15e0ed58da6e4d83a736346cac209ece&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=15e0ed58da6e4d83a736346cac209ece&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 1954:1959
**Comment:**
*Incorrect Condition Logic: `active` becomes true for any nonblank
transform, including missing `:value`, invalid SQL, Jinja, or nondeterministic
functions that validation leaves inactive.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=e2ea1192d68592ea4c2e912817033b69168663e6d0ae6e3cedfb6f9312eaebac&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=e2ea1192d68592ea4c2e912817033b69168663e6d0ae6e3cedfb6f9312eaebac&reaction=dislike'>๐</a>
##########
superset/daos/dataset.py:
##########
@@ -529,6 +560,8 @@ def _override_columns(
}
db.session.add(TableColumn(**{**cleaned, "table_id":
model.id}))
+ cls.clear_dangling_partition_mapping(model, set(incoming_by_name))
Review Comment:
**Suggestion:** The cleanup is called only after `_override_columns`;
`_upsert_columns` can also delete the referenced columns, leaving dangling
partition mapping names in the dataset.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Stale reference`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9b5fc8b851384c7ca45b2ab383b93a0b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9b5fc8b851384c7ca45b2ab383b93a0b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 563:563
**Comment:**
*Stale Reference: The cleanup is called only after `_override_columns`;
`_upsert_columns` can also delete the referenced columns, leaving dangling
partition mapping names in the dataset.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=738fb6b54a228e6ddec6f63f43e4fd06c5c219074fe98daf290ad213b08a3e9e&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43757&comment_hash=738fb6b54a228e6ddec6f63f43e4fd06c5c219074fe98daf290ad213b08a3e9e&reaction=dislike'>๐</a>
--
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]
