codeant-ai-for-open-source[bot] commented on code in PR #42144:
URL: https://github.com/apache/superset/pull/42144#discussion_r3616499234
##########
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:
##########
@@ -889,3 +889,271 @@ async def
test_query_dataset_hidden_from_tools_list_when_metadata_restricted(
assert is_tool_visible_to_current_user(tool) is True
finally:
app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class TestQueryDatasetBracketShorthandNormalization:
+ """QueryDatasetRequest normalizes bracket-shorthand time ranges.
+
+ LLM clients sometimes pass values like '[year]' or '[quarter]' after
+ seeing grain tokens in dashboard filter contexts. The schema validator
+ maps them to the canonical 'Last <unit>' form so get_since_until() can
+ parse them without raising TimeRangeParseFailError.
+ """
Review Comment:
**Suggestion:** The class docstring states that bracket shorthands are
normalized to a canonical `Last <unit>` form, but the added mapping
intentionally uses explicit `DATEADD/DATETIME` expressions for
second/minute/hour. This contradiction will mislead future maintainers and test
readers; update the docstring to reflect the sub-day exception. [docstring
mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Docstring contradicts sub-day DATEADD normalization behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py`
and locate the
`TestQueryDatasetBracketShorthandNormalization` class at line 894.
2. Read its docstring at lines 895โ901, which states that the schema
validator maps
bracket shorthand values to the canonical `'Last <unit>'` form so
`get_since_until()` can
parse them.
3. Scroll down to `test_hour_bracket_normalized`,
`test_minute_bracket_normalized`, and
`test_second_bracket_normalized` at lines 943โ975, which assert that
`[hour]`, `[minute]`,
and `[second]` normalize to explicit `DATEADD(DATETIME('now'), -1, UNIT) :
DATETIME('now')` expressions rather than `'Last hour'`, `'Last minute'`, or
`'Last
second'`.
4. This shows the class-level docstring is technically inaccurate for
sub-day units and
conflicts with the more precise per-test documentation, which can mislead
readers about
the special-case behavior for hour/minute/second brackets.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6bedf5895ddb4199a10eaa64612f606b&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=6bedf5895ddb4199a10eaa64612f606b&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/mcp_service/dataset/tool/test_query_dataset.py
**Line:** 895:901
**Comment:**
*Docstring Mismatch: The class docstring states that bracket shorthands
are normalized to a canonical `Last <unit>` form, but the added mapping
intentionally uses explicit `DATEADD/DATETIME` expressions for
second/minute/hour. This contradiction will mislead future maintainers and test
readers; update the docstring to reflect the sub-day exception.
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%2F42144&comment_hash=352e073c885999f52cd539ea3a81140f039c9c63d82c38558cdc0d15b106cdce&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42144&comment_hash=352e073c885999f52cd539ea3a81140f039c9c63d82c38558cdc0d15b106cdce&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:
##########
@@ -889,3 +889,271 @@ async def
test_query_dataset_hidden_from_tools_list_when_metadata_restricted(
assert is_tool_visible_to_current_user(tool) is True
finally:
app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class TestQueryDatasetBracketShorthandNormalization:
+ """QueryDatasetRequest normalizes bracket-shorthand time ranges.
+
+ LLM clients sometimes pass values like '[year]' or '[quarter]' after
+ seeing grain tokens in dashboard filter contexts. The schema validator
+ maps them to the canonical 'Last <unit>' form so get_since_until() can
+ parse them without raising TimeRangeParseFailError.
+ """
+
+ def test_year_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[year]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_quarter_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[quarter]"}
+ )
+ assert req.time_range == "Last quarter"
+
+ def test_month_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[month]"}
+ )
+ assert req.time_range == "Last month"
+
+ def test_week_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[week]"}
+ )
+ assert req.time_range == "Last week"
+
+ def test_day_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[day]"}
+ )
+ assert req.time_range == "Last day"
+
+ def test_hour_bracket_normalized(self) -> None:
+ """'[hour]' maps to an explicit DATEADD/DATETIME expression.
+
+ 'Last hour' is deliberately not used: get_since_until() resolves its
+ since-expression against 'now' but its default until-expression
+ against 'today' (midnight), so since ends up after until and raises
+ a "From date cannot be larger than to date" error.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[hour]"}
+ )
+ assert req.time_range == "DATEADD(DATETIME('now'), -1, HOUR) :
DATETIME('now')"
+
+ def test_minute_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[minute]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, MINUTE) :
DATETIME('now')"
+ )
+
+ def test_second_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[second]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, SECOND) :
DATETIME('now')"
+ )
+
+ def test_bracket_uppercase_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[YEAR]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_bracket_with_whitespace_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " [year] "}
+ )
+ assert req.time_range == "Last year"
+
+ def test_valid_superset_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "Last 7
days"}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_iso_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {
+ "dataset_id": 1,
+ "metrics": ["count"],
+ "time_range": "2024-01-01 : 2024-12-31",
+ }
+ )
+ assert req.time_range == "2024-01-01 : 2024-12-31"
+
+ def test_non_bracket_value_is_stripped(self) -> None:
+ """Non-bracket values must be trimmed too, not just the lookup key.
+
+ Otherwise leading/trailing whitespace around an otherwise valid
+ relative range (e.g. from an LLM) would propagate to
+ get_since_until() and could cause avoidable parse failures.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " Last 7
days "}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_none_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": None}
+ )
+ assert req.time_range is None
+
+
[email protected]
+async def test_query_dataset_bracket_year_resolves_without_parse_error(
+ mcp_server: FastMCP,
+) -> None:
+ """'[year]' as time_range must not raise TimeRangeParseFailError.
+
+ Regression test for SC-113648: LLM clients passing '[year]' verbatim
+ triggered TimeRangeParseFailError because the raw token was forwarded to
+ parse_human_datetime() without normalization. The schema validator now
+ maps it to 'Last year' before the query context is built.
+ """
+ dataset = _make_dataset(main_dttm_col="order_date")
+ result_data = _mock_command_result()
+ captured_queries: list[dict[str, Any]] = []
+
+ def capture_create(**kwargs):
+ captured_queries.extend(kwargs.get("queries", []))
+ return MagicMock()
+
+ with (
+ patch.object(
+ query_dataset_module,
+ "resolve_dataset",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.validate",
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.run",
+ return_value=result_data,
+ ),
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory.create",
+ side_effect=capture_create,
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "query_dataset",
+ {
+ "request": {
+ "dataset_id": 1,
+ "metrics": ["count"],
+ "time_range": "[year]",
+ }
+ },
+ )
+
+ data = json.loads(result.content[0].text)
+ # Must succeed (no error_type) and forward the normalized value
+ assert "error_type" not in data or data.get("error_type") is None
+ assert len(captured_queries) == 1
+ temporal_filters = [
+ f for f in captured_queries[0]["filters"] if f["op"] ==
"TEMPORAL_RANGE"
+ ]
+ assert len(temporal_filters) == 1
+ assert temporal_filters[0]["val"] == "Last year"
+
+
[email protected]
+async def test_query_dataset_bracket_hour_resolves_without_parse_error(
+ mcp_server: FastMCP,
+) -> None:
+ """'[hour]' as time_range must not raise TimeRangeParseFailError.
+
+ Regression test for SC-113648. Unlike the other bracket shorthands,
+ '[hour]' does not normalize to 'Last hour': get_since_until() resolves
+ that expression's since-clause against 'now' but its until-clause
+ against 'today' (midnight), which raises "From date cannot be larger
+ than to date" for any sub-day unit. The schema validator instead maps
+ '[hour]' to an explicit DATEADD/DATETIME range that resolves both ends
+ against 'now'.
+ """
+ dataset = _make_dataset(main_dttm_col="order_date")
+ result_data = _mock_command_result()
+ captured_queries: list[dict[str, Any]] = []
+
+ def capture_create(**kwargs):
+ captured_queries.extend(kwargs.get("queries", []))
+ return MagicMock()
+
+ with (
+ patch.object(
+ query_dataset_module,
+ "resolve_dataset",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.validate",
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.run",
+ return_value=result_data,
+ ),
Review Comment:
**Suggestion:** This second regression test has the same issue: by patching
validation and run on the chart data command, it skips the layer that would
raise temporal parse errors, so the test cannot truly verify that the
normalized hour expression is parse-safe. Remove or narrow these mocks so the
parse path is exercised. [logic error]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ Hour-regression test cannot detect parser regressions.
- โ ๏ธ MCP hour-range handling less reliably guarded in tests.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py`
and locate
`test_query_dataset_bracket_hour_resolves_without_parse_error` starting at
line 1098.
2. Examine its `with` block at lines 1119โ1135:
`superset.commands.chart.data.get_data_command.ChartDataCommand.validate` and
`ChartDataCommand.run` are patched (lines 1125โ1131), and
`QueryContextFactory.create` is
patched to `capture_create`, so the real chart-command pipeline and temporal
parser are
never invoked.
3. The test then asserts that no `error_type` is present in the JSON payload
and that the
captured temporal filter value equals `"DATEADD(DATETIME('now'), -1, HOUR) :
DATETIME('now')"`, but those assertions are against data produced before any
real parsing
logic would run.
4. If the real Superset parser or command layer were later changed to
mishandle this
DATEADD/DATETIME expression (for example, raising `TimeRangeParseFailError`
even for the
normalized hour range), the test would still pass because the patched
validate/run/create
short-circuit the actual parsing path, so CI would not catch the regression.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=87ec1b3e73214c8c83afed1c204cc579&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=87ec1b3e73214c8c83afed1c204cc579&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/mcp_service/dataset/tool/test_query_dataset.py
**Line:** 1125:1131
**Comment:**
*Logic Error: This second regression test has the same issue: by
patching validation and run on the chart data command, it skips the layer that
would raise temporal parse errors, so the test cannot truly verify that the
normalized hour expression is parse-safe. Remove or narrow these mocks so the
parse path is exercised.
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%2F42144&comment_hash=e83b035d421c7c6f37ee3664b13f7fd82f1f45876713af4e0d5f92309a1f232d&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42144&comment_hash=e83b035d421c7c6f37ee3664b13f7fd82f1f45876713af4e0d5f92309a1f232d&reaction=dislike'>๐</a>
##########
tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py:
##########
@@ -889,3 +889,271 @@ async def
test_query_dataset_hidden_from_tools_list_when_metadata_restricted(
assert is_tool_visible_to_current_user(tool) is True
finally:
app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class TestQueryDatasetBracketShorthandNormalization:
+ """QueryDatasetRequest normalizes bracket-shorthand time ranges.
+
+ LLM clients sometimes pass values like '[year]' or '[quarter]' after
+ seeing grain tokens in dashboard filter contexts. The schema validator
+ maps them to the canonical 'Last <unit>' form so get_since_until() can
+ parse them without raising TimeRangeParseFailError.
+ """
+
+ def test_year_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[year]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_quarter_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[quarter]"}
+ )
+ assert req.time_range == "Last quarter"
+
+ def test_month_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[month]"}
+ )
+ assert req.time_range == "Last month"
+
+ def test_week_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[week]"}
+ )
+ assert req.time_range == "Last week"
+
+ def test_day_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[day]"}
+ )
+ assert req.time_range == "Last day"
+
+ def test_hour_bracket_normalized(self) -> None:
+ """'[hour]' maps to an explicit DATEADD/DATETIME expression.
+
+ 'Last hour' is deliberately not used: get_since_until() resolves its
+ since-expression against 'now' but its default until-expression
+ against 'today' (midnight), so since ends up after until and raises
+ a "From date cannot be larger than to date" error.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[hour]"}
+ )
+ assert req.time_range == "DATEADD(DATETIME('now'), -1, HOUR) :
DATETIME('now')"
+
+ def test_minute_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[minute]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, MINUTE) :
DATETIME('now')"
+ )
+
+ def test_second_bracket_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[second]"}
+ )
+ assert (
+ req.time_range == "DATEADD(DATETIME('now'), -1, SECOND) :
DATETIME('now')"
+ )
+
+ def test_bracket_uppercase_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "[YEAR]"}
+ )
+ assert req.time_range == "Last year"
+
+ def test_bracket_with_whitespace_normalized(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " [year] "}
+ )
+ assert req.time_range == "Last year"
+
+ def test_valid_superset_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": "Last 7
days"}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_iso_range_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {
+ "dataset_id": 1,
+ "metrics": ["count"],
+ "time_range": "2024-01-01 : 2024-12-31",
+ }
+ )
+ assert req.time_range == "2024-01-01 : 2024-12-31"
+
+ def test_non_bracket_value_is_stripped(self) -> None:
+ """Non-bracket values must be trimmed too, not just the lookup key.
+
+ Otherwise leading/trailing whitespace around an otherwise valid
+ relative range (e.g. from an LLM) would propagate to
+ get_since_until() and could cause avoidable parse failures.
+ """
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": " Last 7
days "}
+ )
+ assert req.time_range == "Last 7 days"
+
+ def test_none_unchanged(self) -> None:
+ from superset.mcp_service.dataset.schemas import QueryDatasetRequest
+
+ req = QueryDatasetRequest.model_validate(
+ {"dataset_id": 1, "metrics": ["count"], "time_range": None}
+ )
+ assert req.time_range is None
+
+
[email protected]
+async def test_query_dataset_bracket_year_resolves_without_parse_error(
+ mcp_server: FastMCP,
+) -> None:
+ """'[year]' as time_range must not raise TimeRangeParseFailError.
+
+ Regression test for SC-113648: LLM clients passing '[year]' verbatim
+ triggered TimeRangeParseFailError because the raw token was forwarded to
+ parse_human_datetime() without normalization. The schema validator now
+ maps it to 'Last year' before the query context is built.
+ """
+ dataset = _make_dataset(main_dttm_col="order_date")
+ result_data = _mock_command_result()
+ captured_queries: list[dict[str, Any]] = []
+
+ def capture_create(**kwargs):
+ captured_queries.extend(kwargs.get("queries", []))
+ return MagicMock()
+
+ with (
+ patch.object(
+ query_dataset_module,
+ "resolve_dataset",
+ return_value=dataset,
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.validate",
+ ),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand.run",
+ return_value=result_data,
+ ),
Review Comment:
**Suggestion:** This regression test claims to verify that bracket shorthand
no longer triggers time-range parse failures, but mocking both command
validation and execution bypasses the code path where temporal parsing errors
are normally raised. As written, the test can still pass even if parsing is
broken, so it does not actually protect the reported regression; avoid mocking
out the parsing path (or explicitly assert the parser-facing behavior) so
failures surface. [logic error]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ Year-regression test may miss future parsing regressions.
- โ ๏ธ MCP query_dataset tool parsing less protected in CI.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py`
and locate
`test_query_dataset_bracket_year_resolves_without_parse_error` starting at
line 1038.
2. Inspect the `with` block in that test at lines 1056โ1072, where
`superset.commands.chart.data.get_data_command.ChartDataCommand.validate` and
`ChartDataCommand.run` are patched (lines 1062โ1068) and
`QueryContextFactory.create` is
patched with `side_effect=capture_create`.
3. Note that these patches mean none of the real Superset validation or
query-context
construction code runs when the MCP `query_dataset` tool is invoked;
temporal parsing
(including raising `TimeRangeParseFailError`) would normally happen inside
those real
implementations, not inside this test file.
4. If the production implementation of `ChartDataCommand.validate` or the
real
query-context factory in `superset/commands/chart/data/get_data_command.py`
were changed
to mis-handle normalized ranges (e.g., still raising
`TimeRangeParseFailError` for `"Last
year"`), this test would continue to pass because it only asserts on the
normalized filter
value captured from the patched `create` and on the mocked `result_data`,
never exercising
the real parser path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=895cae2e2e3d4b249a3a25a415cd7705&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=895cae2e2e3d4b249a3a25a415cd7705&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/mcp_service/dataset/tool/test_query_dataset.py
**Line:** 1062:1068
**Comment:**
*Logic Error: This regression test claims to verify that bracket
shorthand no longer triggers time-range parse failures, but mocking both
command validation and execution bypasses the code path where temporal parsing
errors are normally raised. As written, the test can still pass even if parsing
is broken, so it does not actually protect the reported regression; avoid
mocking out the parsing path (or explicitly assert the parser-facing behavior)
so failures surface.
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%2F42144&comment_hash=31864f8953b382f6a64ba31e4480c7cd3413bf622342dfeb5f740cea89309130&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42144&comment_hash=31864f8953b382f6a64ba31e4480c7cd3413bf622342dfeb5f740cea89309130&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]
