codeant-ai-for-open-source[bot] commented on code in PR #41456:
URL: https://github.com/apache/superset/pull/41456#discussion_r3531000933
##########
superset/semantic_layers/mapper.py:
##########
@@ -309,18 +309,43 @@ def map_query_object(query_object: ValidatedQueryObject)
-> list[SemanticQuery]:
metrics = [all_metrics[metric] for metric in (query_object.metrics or [])]
grain = _convert_time_grain(query_object.extras.get("time_grain_sqla"))
- dimensions = [
- dimension
- for dimension in semantic_view.dimensions
- if dimension.name in normalized_columns
- and (
- # if a grain is specified, only include the time dimension if its
grain
- # matches the requested grain
- grain is None
- or dimension.name != query_object.granularity
- or dimension.grain == grain
+ time_axis_column = _get_time_axis_column(query_object, all_dimensions)
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local value so the selected time-axis type is clearly declared. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added Python code and the local variable is clearly
annotatable as `str | None` from the called function signature, but no type
hint is present. That matches the type-hinting rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1dcd68bce2234912a5a8314724988036&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=1dcd68bce2234912a5a8314724988036&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/mapper.py
**Line:** 312:312
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local value so the selected time-axis type is clearly declared.
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%2F41456&comment_hash=125fac8b2b9f75f0e9783f9b8eb00c9e52aab0fb22714545e7a910794dad9471&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=125fac8b2b9f75f0e9783f9b8eb00c9e52aab0fb22714545e7a910794dad9471&reaction=dislike'>👎</a>
##########
superset/semantic_layers/mapper.py:
##########
@@ -990,15 +1059,20 @@ def _validate_granularity(query_object:
ValidatedQueryObject) -> None:
Make sure time column and time grain are valid.
"""
semantic_view = query_object.datasource.implementation
- dimension_names = {dimension.name for dimension in
semantic_view.dimensions}
+ all_dimensions = {
+ dimension.name: dimension for dimension in semantic_view.dimensions
+ }
Review Comment:
**Suggestion:** Add an explicit dictionary type annotation for this new
dimensions mapping in validation logic. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is newly added code in a modified Python file, and the mapping is
clearly annotatable as `dict[str, Dimension]` but lacks an explicit type hint.
That satisfies the custom rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=480d8661a8834a6a970407e3cdd29d53&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=480d8661a8834a6a970407e3cdd29d53&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/mapper.py
**Line:** 1062:1064
**Comment:**
*Custom Rule: Add an explicit dictionary type annotation for this new
dimensions mapping in validation logic.
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%2F41456&comment_hash=9077332c41fa0296ef3e028a930b8b16ed391cc7506f14a32e20c59d0c11d6d0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=9077332c41fa0296ef3e028a930b8b16ed391cc7506f14a32e20c59d0c11d6d0&reaction=dislike'>👎</a>
##########
superset/semantic_layers/mapper.py:
##########
@@ -309,18 +309,43 @@ def map_query_object(query_object: ValidatedQueryObject)
-> list[SemanticQuery]:
metrics = [all_metrics[metric] for metric in (query_object.metrics or [])]
grain = _convert_time_grain(query_object.extras.get("time_grain_sqla"))
- dimensions = [
- dimension
- for dimension in semantic_view.dimensions
- if dimension.name in normalized_columns
- and (
- # if a grain is specified, only include the time dimension if its
grain
- # matches the requested grain
- grain is None
- or dimension.name != query_object.granularity
- or dimension.grain == grain
+ time_axis_column = _get_time_axis_column(query_object, all_dimensions)
+ # A semantic view can expose multiple Dimension variants per name (one per
+ # supported time grain). Pick exactly one variant per selected column:
+ # for the time-axis column we honor the user's grain selection, falling
+ # back to the raw / no-grain variant when no exact match exists and then
+ # to any available variant so the axis is never silently dropped; for
+ # every other selected column we prefer the raw variant and otherwise
+ # take any available variant.
+ dimensions: list[Dimension] = []
+ seen_non_axis: dict[str, Dimension] = {}
+ axis_variants: list[Dimension] = []
+ axis_match: Dimension | None = None
+ for dimension in semantic_view.dimensions:
+ if dimension.name not in normalized_columns:
+ continue
+ if dimension.name == time_axis_column:
+ axis_variants.append(dimension)
+ if axis_match is None and dimension.grain == grain:
+ axis_match = dimension
+ continue
+ existing = seen_non_axis.get(dimension.name)
+ if existing is None or (existing.grain is not None and dimension.grain
is None):
+ seen_non_axis[dimension.name] = dimension
+
+ if axis_match is not None:
+ dimensions.append(axis_match)
+ elif axis_variants:
+ # No variant matches the requested grain. Prefer the raw (grain=None)
+ # variant; otherwise pick a deterministic fallback so the axis stays
+ # on the query instead of being silently dropped.
+ raw_variant = next((v for v in axis_variants if v.grain is None), None)
Review Comment:
**Suggestion:** Add a concrete type annotation to this fallback dimension
variable to comply with the type-hinting requirement for relevant new locals.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable is inferable as `Dimension | None` and is relevant
new Python code, but it is left untyped. That is a real violation of the
type-hints rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=45aabd4634684777af4a00d483f0143d&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=45aabd4634684777af4a00d483f0143d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/mapper.py
**Line:** 342:342
**Comment:**
*Custom Rule: Add a concrete type annotation to this fallback dimension
variable to comply with the type-hinting requirement for relevant new locals.
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%2F41456&comment_hash=cff372f853944e7d1ef0b0a66eaa4ce14580477929c852150b10fef240db4d82&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=cff372f853944e7d1ef0b0a66eaa4ce14580477929c852150b10fef240db4d82&reaction=dislike'>👎</a>
##########
superset/semantic_layers/mapper.py:
##########
@@ -990,15 +1059,20 @@ def _validate_granularity(query_object:
ValidatedQueryObject) -> None:
Make sure time column and time grain are valid.
"""
semantic_view = query_object.datasource.implementation
- dimension_names = {dimension.name for dimension in
semantic_view.dimensions}
+ all_dimensions = {
+ dimension.name: dimension for dimension in semantic_view.dimensions
+ }
+ dimension_names = set(all_dimensions.keys())
- if time_column := query_object.granularity:
- if time_column not in dimension_names:
- raise ValueError(
- "The time column must be defined in the Semantic View
dimensions."
- )
+ if (legacy_time_column := query_object.granularity) and (
+ legacy_time_column not in dimension_names
+ ):
+ raise ValueError(
+ "The time column must be defined in the Semantic View dimensions."
+ )
if time_grain := query_object.extras.get("time_grain_sqla"):
+ time_column = _get_time_axis_column(query_object, all_dimensions)
Review Comment:
**Suggestion:** Annotate this new local variable with its optional string
type to satisfy the rule requiring type hints in modified Python code.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new local variable is inferable as `str | None` from
`_get_time_axis_column`, so it can be annotated but is not. This is a genuine
type-hint omission under the rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f2e8dba1e684e139a2699168d83d592&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=9f2e8dba1e684e139a2699168d83d592&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/mapper.py
**Line:** 1075:1075
**Comment:**
*Custom Rule: Annotate this new local variable with its optional string
type to satisfy the rule requiring type hints in modified Python code.
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%2F41456&comment_hash=fb082385e2e27836cbd2bfcb13e21f92defd2d8ea32df338f206ac27e6f4aa76&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=fb082385e2e27836cbd2bfcb13e21f92defd2d8ea32df338f206ac27e6f4aa76&reaction=dislike'>👎</a>
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -655,6 +655,118 @@ def test_semantic_view_data(
assert data["offset"] == 0
[email protected]
+def mock_grain_variant_dimensions() -> list[Dimension]:
+ """Time column exposed as multiple Dimension variants, one per grain."""
+ base = {
+ "id": "orders.created_at",
+ "name": "created_at",
+ "type": pa.timestamp("us"),
+ "definition": "orders.created_at",
+ "description": "Order timestamp",
+ }
+ return [
+ Dimension(**base, grain=Grains.HOUR),
+ Dimension(**base, grain=Grains.DAY),
+ Dimension(**base, grain=Grains.MONTH),
+ Dimension(
+ id="products.category",
+ name="category",
+ type=pa.utf8(),
+ definition="products.category",
+ description="Product category",
+ grain=None,
+ ),
+ ]
+
+
+def test_semantic_view_columns_dedupes_grain_variants(
+ mock_grain_variant_dimensions: list[Dimension],
+) -> None:
+ """Multiple grain variants of the same time column collapse to one
column."""
+ impl = MagicMock()
Review Comment:
**Suggestion:** Annotate this new local mock variable with its intended type
so the added code consistently follows the type-hint rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added local variable without a type annotation. Since the
code can clearly be annotated, it violates the requirement to add type hints to
relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d0847bc4cd9d44188910389b49c54a66&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=d0847bc4cd9d44188910389b49c54a66&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/semantic_layers/models_test.py
**Line:** 687:687
**Comment:**
*Custom Rule: Annotate this new local mock variable with its intended
type so the added code consistently follows the type-hint rule.
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%2F41456&comment_hash=b157222e44bd733a9df0138b9ac6e47089326c561129c8525e5efa7ceb003eac&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=b157222e44bd733a9df0138b9ac6e47089326c561129c8525e5efa7ceb003eac&reaction=dislike'>👎</a>
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -655,6 +655,118 @@ def test_semantic_view_data(
assert data["offset"] == 0
[email protected]
+def mock_grain_variant_dimensions() -> list[Dimension]:
+ """Time column exposed as multiple Dimension variants, one per grain."""
+ base = {
+ "id": "orders.created_at",
+ "name": "created_at",
+ "type": pa.timestamp("us"),
+ "definition": "orders.created_at",
+ "description": "Order timestamp",
+ }
Review Comment:
**Suggestion:** Add an explicit type annotation to this dictionary variable
so the new test code satisfies the type-hint requirement for relevant
variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added local variable is an unannotated dictionary in new Python
code. The rule requires type hints on relevant variables that can be annotated,
so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=701b465062a04cb8a1d4291b47238700&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=701b465062a04cb8a1d4291b47238700&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/semantic_layers/models_test.py
**Line:** 661:667
**Comment:**
*Custom Rule: Add an explicit type annotation to this dictionary
variable so the new test code satisfies the type-hint requirement for relevant
variables.
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%2F41456&comment_hash=cd7431f9656bc849d4e72d35760d67fafd8fc5eb79fff4c97f01de9ecdebb359&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=cd7431f9656bc849d4e72d35760d67fafd8fc5eb79fff4c97f01de9ecdebb359&reaction=dislike'>👎</a>
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -655,6 +655,118 @@ def test_semantic_view_data(
assert data["offset"] == 0
[email protected]
+def mock_grain_variant_dimensions() -> list[Dimension]:
+ """Time column exposed as multiple Dimension variants, one per grain."""
+ base = {
+ "id": "orders.created_at",
+ "name": "created_at",
+ "type": pa.timestamp("us"),
+ "definition": "orders.created_at",
+ "description": "Order timestamp",
+ }
+ return [
+ Dimension(**base, grain=Grains.HOUR),
+ Dimension(**base, grain=Grains.DAY),
+ Dimension(**base, grain=Grains.MONTH),
+ Dimension(
+ id="products.category",
+ name="category",
+ type=pa.utf8(),
+ definition="products.category",
+ description="Product category",
+ grain=None,
+ ),
+ ]
+
+
+def test_semantic_view_columns_dedupes_grain_variants(
+ mock_grain_variant_dimensions: list[Dimension],
+) -> None:
+ """Multiple grain variants of the same time column collapse to one
column."""
+ impl = MagicMock()
+ impl.get_dimensions.return_value = mock_grain_variant_dimensions
+ view = SemanticView()
+
+ with patch.object(
+ SemanticView,
+ "implementation",
+ new_callable=lambda: property(lambda s: impl),
+ ):
+ columns = view.columns
+ assert [c.column_name for c in columns] == ["created_at", "category"]
+ assert columns[0].is_dttm is True
+ assert view.column_names == ["created_at", "category"]
+
+
+def test_semantic_view_get_time_grains_dedupes_across_dimensions(
+ mock_grain_variant_dimensions: list[Dimension],
+) -> None:
+ """Grains shared across multiple time dimensions are returned once each."""
+ extra_dim = Dimension(
+ id="shipments.shipped_at",
+ name="shipped_at",
+ type=pa.timestamp("us"),
+ definition="shipments.shipped_at",
+ description=None,
+ grain=Grains.DAY,
+ )
Review Comment:
**Suggestion:** Add a concrete type annotation to this newly introduced
variable to comply with the rule requiring type hints on relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly introduced local variable is unannotated even though its type is
clear from context. That fits the type-hint rule for relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=63e7da5492e94054a21736463cc672c2&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=63e7da5492e94054a21736463cc672c2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/semantic_layers/models_test.py
**Line:** 706:713
**Comment:**
*Custom Rule: Add a concrete type annotation to this newly introduced
variable to comply with the rule requiring type hints on relevant variables.
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%2F41456&comment_hash=fb16fc51a05e7507bea7f61ec5c749fb4028a29ecc11894837871261127f1155&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=fb16fc51a05e7507bea7f61ec5c749fb4028a29ecc11894837871261127f1155&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]
