codeant-ai-for-open-source[bot] commented on code in PR #41456:
URL: https://github.com/apache/superset/pull/41456#discussion_r3531045820
##########
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)
+ dimensions.append(
+ raw_variant
+ if raw_variant is not None
+ else min(axis_variants, key=lambda v: v.grain.name if v.grain else
"")
)
Review Comment:
**Suggestion:** This fallback branch is effectively unreachable in
production because `get_results()` always runs `validate_query_object()` first,
and `_validate_granularity` still rejects unsupported `time_grain_sqla` values
before `map_query_object` runs. As a result, the new “keep axis with
deterministic fallback” behavior does not actually execute for real requests.
Align validation with this mapping contract (allow fallback) or remove the
fallback path to avoid contradictory behavior. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unsupported time grains abort semantic view queries with errors.
- ⚠️ Axis grain mismatch fallback logic never runs in production.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Set up a semantic view where the time-axis Dimension (e.g. `order_date`)
exposes only
grained variants such as `Grains.HOUR` and `Grains.DAY`, matching the
`variants` set built
in `tests/unit_tests/semantic_layers/mapper_test.py:76-80` for `order_date`.
2. Create a `ValidatedQueryObject` (using the type from
`superset/semantic_layers/mapper.py:86-99`) with `datasource.implementation`
pointing to
that view, `columns=["order_date"]`, `granularity="order_date"`, and
`extras={"time_grain_sqla": "P1M"}` to request a month grain that is not
among the
Dimension’s supported grains, as done in
`test_map_query_object_falls_back_when_no_grain_variant_matches` at
`tests/unit_tests/semantic_layers/mapper_test.py:96-102`.
3. Execute an Explore query so `QueryContextProcessor.get_query_result()` at
`superset/common/query_context_processor.py:248-256` calls the datasource’s
`get_query_result()`, which for semantic views is
`SemanticView.get_query_result()` at
`superset/semantic_layers/models.py:34-35`, and this delegates to
`get_results(query_object)` in `superset/semantic_layers/mapper.py:100-122`.
4. In `get_results()`, `validate_query_object()` at
`superset/semantic_layers/mapper.py:990-1011` invokes
`_validate_granularity()` at lines
1057-1089. `time_column` is resolved to `"order_date"`,
`supported_time_grains` is built
from the semantic view’s Dimensions for `order_date` (e.g. `{Grains.HOUR,
Grains.DAY}`),
and `_convert_time_grain("P1M")` at lines 976-987 returns a `Grain` value
not in that set.
The membership check at lines 1082-1087 fails, raising `ValueError("The time
grain is not
supported for the time column in the Semantic View.")` before
`map_query_object()` at
lines 289-379 runs, so the axis grain-mismatch fallback block at lines
338-347 (`elif
axis_variants: ... dimensions.append(...)`) never executes for real requests
and only runs
in unit tests that call `map_query_object()` directly, leading to a mismatch
between
validation behavior and the mapping contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ee7b1d93d74548959999fbc6c7d84f5b&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=ee7b1d93d74548959999fbc6c7d84f5b&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:** 338:347
**Comment:**
*Api Mismatch: This fallback branch is effectively unreachable in
production because `get_results()` always runs `validate_query_object()` first,
and `_validate_granularity` still rejects unsupported `time_grain_sqla` values
before `map_query_object` runs. As a result, the new “keep axis with
deterministic fallback” behavior does not actually execute for real requests.
Align validation with this mapping contract (allow fallback) or remove the
fallback path to avoid contradictory 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%2F41456&comment_hash=2d82139a014beee1da52ccf5e764c91427d3ab65bba02ba25b584e2a14f9baf1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=2d82139a014beee1da52ccf5e764c91427d3ab65bba02ba25b584e2a14f9baf1&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:** The new validation path rejects any request where a time
grain is set and `_get_time_axis_column` returns `None`, but
`_get_time_axis_column` now intentionally returns `None` when multiple temporal
columns are selected (ambiguity case). That makes valid modern queries fail
with a hard `ValueError` instead of using the intended fallback behavior. Only
raise when there is truly no temporal candidate, or disambiguate via available
query metadata (eg temporal filter subject) before failing. [incorrect
condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Semantic view Explore queries fail with ambiguous time axes.
- ⚠️ Users cannot run multi-temporal time-grain visualizations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a semantic view implementation with multiple temporal
Dimensions (for
example, `created_at` and `shipped_at` of type `pa.timestamp("us")`),
following the
pattern in `tests/unit_tests/semantic_layers/mapper_test.py:144-157` where
two timestamp
dimensions are defined.
2. Instantiate a `ValidatedQueryObject` (class defined at
`superset/semantic_layers/mapper.py:86-99`) whose
`datasource.implementation` is that
semantic view, with `columns=["created_at", "shipped_at"]`,
`granularity=None`, and
`extras={"time_grain_sqla": "P1D"}` so a time grain is selected but the
legacy
`granularity` field is empty.
3. Call `validate_query_object(query_object)` (function at
`superset/semantic_layers/mapper.py:990-1011`) directly, or trigger an
Explore query so
that `QueryContextProcessor.get_query_result()` at
`superset/common/query_context_processor.py:248-256` delegates to the
datasource’s
`get_query_result()`, which for semantic views is
`SemanticView.get_query_result()` at
`superset/semantic_layers/models.py:34-35`, and then to `get_results()` at
`superset/semantic_layers/mapper.py:100-122`.
4. Inside `validate_query_object()`, `_validate_granularity()` at
`superset/semantic_layers/mapper.py:1057-1089` calls
`_get_time_axis_column(query_object,
all_dimensions)` at line 1075. Because there are multiple temporal columns
and
`granularity` is unset, `_get_time_axis_column()` (implementation at lines
932-973 and
behavior verified by
`test_get_time_axis_column_returns_none_on_multiple_temporal_columns`
in `tests/unit_tests/semantic_layers/mapper_test.py:134-161`) returns
`None`, so the `if
not time_column:` check at line 1076 raises `ValueError("A time column must
be specified
when a time grain is provided.")` and rejects the query, even though
`map_query_object()`
at lines 289-379 is designed to handle this ambiguity by falling back to raw
variants when
no unique time axis can be identified.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=018e33d60f4a47988b1ef649de320061&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=018e33d60f4a47988b1ef649de320061&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:1079
**Comment:**
*Incorrect Condition Logic: The new validation path rejects any request
where a time grain is set and `_get_time_axis_column` returns `None`, but
`_get_time_axis_column` now intentionally returns `None` when multiple temporal
columns are selected (ambiguity case). That makes valid modern queries fail
with a hard `ValueError` instead of using the intended fallback behavior. Only
raise when there is truly no temporal candidate, or disambiguate via available
query metadata (eg temporal filter subject) before failing.
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=ceeb00047e4c884247d1a51bc6cebe8626622e9eea3229ad2815499420681244&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41456&comment_hash=ceeb00047e4c884247d1a51bc6cebe8626622e9eea3229ad2815499420681244&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]
