codeant-ai-for-open-source[bot] commented on code in PR #41611: URL: https://github.com/apache/superset/pull/41611#discussion_r3531365489
########## superset/mcp_service/semantic_layer/tool/get_table.py: ########## @@ -0,0 +1,511 @@ +# 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. + +"""MCP tool: get_table + +Query a data source (built-in dataset or external semantic view) using Review Comment: **Suggestion:** When `time_range` is provided for an external semantic view that has no datetime dimensions, the code only adds a warning and executes an unfiltered query. This returns incorrect data for a time-bounded request and violates the request contract that `time_range` requires a datetime dimension. Return a validation error instead of silently dropping the time filter. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Semantic get_table tool ignores time_range on some views. - ⚠️ LLM clients may analyze unintended, unbounded historical data. - ⚠️ Behavior contradicts GetTableRequest time_range contract in schemas. - ⚠️ External semantic views without datetime dimensions affected directly. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The MCP tool entrypoint `get_table()` in `superset/mcp_service/semantic_layer/tool/get_table.py` (decorated at lines 394-401 and implemented below that in the same file) accepts a `GetTableRequest` defined in `superset/mcp_service/semantic_layer/schemas.py:151-216` with fields `view_id`, `time_range`, and optional `time_column`. 2. When the request targets an external semantic view (i.e. `dataset_id` is None and `view_id` is set), `get_table()` delegates to `_run_get_table_query()` at `superset/mcp_service/semantic_layer/tool/get_table.py:293-371`, which in turn calls `_resolve_external_view(request)` at `superset/mcp_service/semantic_layer/tool/get_table.py:128-178`. 3. In `_resolve_external_view()` at lines 150-25, the block at lines 11-20 of the snippet (`superset/mcp_service/semantic_layer/tool/get_table.py:160-169`) handles the case where `request.time_range` is provided but `request.time_column` is None by computing `dttm_cols = [c for c in view.columns if c.is_dttm]`; when the `SemanticView` has no datetime dimensions (`dttm_cols` is empty), it appends a warning `"time_range provided but no datetime dimension found; time filter will not be applied."` and sets `time_col = None` instead of returning an error, even though `GetTableRequest.time_range` is documented in `superset/mcp_service/semantic_layer/schemas.py:183-188` as "Requires a datetime dimension." 4. Back in `_run_get_table_query()`, the query dict is constructed by `_build_query_dict()` at `superset/mcp_service/semantic_layer/tool/get_table.py:202-228`; because `time_col` was set to `None`, the conditional at lines 210-213 does not append a `TEMPORAL_RANGE` filter despite `request.time_range` being non-null, so the query executed via `QueryContextFactory` and `ChartDataCommand` (lines 332-343) returns unfiltered data for the entire view, silently ignoring the caller’s requested `time_range` instead of failing fast with a validation error. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d10d2bef559140e29cc874925d8bf2ef&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=d10d2bef559140e29cc874925d8bf2ef&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/mcp_service/semantic_layer/tool/get_table.py **Line:** 13:20 **Comment:** *Logic Error: When `time_range` is provided for an external semantic view that has no datetime dimensions, the code only adds a warning and executes an unfiltered query. This returns incorrect data for a time-bounded request and violates the request contract that `time_range` requires a datetime dimension. Return a validation error instead of silently dropping the time filter. 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%2F41611&comment_hash=dd2887ef5822baada392d889ccbd15ad285198777703bb24ff5ebc9d8bbec79d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=dd2887ef5822baada392d889ccbd15ad285198777703bb24ff5ebc9d8bbec79d&reaction=dislike'>👎</a> ########## superset/mcp_service/utils/response_utils.py: ########## @@ -161,3 +164,49 @@ def add_extracted_field( def build(self) -> Dict[str, str]: """Return the omission metadata dict.""" return dict(self._fields) + + +def format_data_columns( + data: list[dict[str, Any]], raw_columns: list[str] +) -> list[DataColumn]: + """Build column metadata from query result data. + + Caps statistics at 5000 rows to avoid O(rows*cols) overhead on large + result sets. + """ + # Local import breaks the chart.schemas ↔ response_utils circular dependency. + from superset.mcp_service.chart.schemas import DataColumn # noqa: PLC0415 + + stats_rows = data[:5000] + columns_meta: list[DataColumn] = [] + for col_name in raw_columns: + sample_values = [ + row.get(col_name) for row in data[:3] if row.get(col_name) is not None + ] + data_type = "string" + if sample_values: + if all(isinstance(v, bool) for v in sample_values): + data_type = "boolean" + elif all(isinstance(v, (int, float)) for v in sample_values): + data_type = "numeric" + + null_count = 0 + unique_vals: set[str] = set() + for row in stats_rows: + val = row.get(col_name) + if val is None: + null_count += 1 + else: + unique_vals.add(str(val)) Review Comment: **Suggestion:** The function caps column statistics to the first 5000 rows but still returns `null_count` and `unique_count` as if they were full-result totals. For result sets larger than 5000 rows this produces incorrect metadata and can mislead downstream logic. Either compute counts on the full dataset or explicitly mark these values as sampled/approximate in the response contract. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ QueryDataset MCP responses expose undercounted column statistics. - ⚠️ Semantic get_table responses misreport null and unique counts. - ⚠️ LLM agents may overtrust inaccurate column metadata summaries. - ⚠️ Inconsistency between DataColumn schema docs and implementation. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The helper `format_data_columns()` in `superset/mcp_service/utils/response_utils.py:169-212` builds `DataColumn` metadata for MCP responses and is imported by both `superset/mcp_service/dataset/tool/query_dataset.py:53` and `superset/mcp_service/semantic_layer/tool/get_table.py:50`. 2. In the dataset MCP tool, after executing the query, `query_dataset()` formats its response at `superset/mcp_service/dataset/tool/query_dataset.py:330-66` by reading `data = query_result.get("data", [])` and `raw_columns = query_result.get("colnames", [])` and then calling `columns_meta = format_data_columns(data, raw_columns)` to populate `QueryDatasetResponse.columns` (schema at `superset/mcp_service/dataset/schemas.py:2-31`). 3. In the semantic-layer MCP tool, `_build_response()` at `superset/mcp_service/semantic_layer/tool/get_table.py:231-290` performs the same pattern, calling `format_data_columns(data, raw_columns)` to fill `GetTableResponse.columns` for `get_table()` responses. 4. Inside `format_data_columns()` at `superset/mcp_service/utils/response_utils.py:180-200`, the function caps statistics to the first 5000 rows with `stats_rows = data[:5000]` and computes `null_count` and `unique_vals` only over `stats_rows`, but the `DataColumn` schema in `superset/mcp_service/chart/schemas.py:2104-2115` describes `null_count` as "Number of null values" and `unique_count` as "Number of unique values" without noting sampling; when callers like `query_dataset()` or `get_table()` return more than 5000 rows (e.g., by setting `row_limit` above 5000 in `GetTableRequest` at `superset/mcp_service/semantic_layer/schemas.py:197-202`), the reported `null_count` and `unique_count` values systematically undercount the true values for the returned data, exposing misleading column metadata to MCP clients. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ab95e89af0214091b3e0dd9abd96eb4c&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=ab95e89af0214091b3e0dd9abd96eb4c&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/mcp_service/utils/response_utils.py **Line:** 180:200 **Comment:** *Logic Error: The function caps column statistics to the first 5000 rows but still returns `null_count` and `unique_count` as if they were full-result totals. For result sets larger than 5000 rows this produces incorrect metadata and can mislead downstream logic. Either compute counts on the full dataset or explicitly mark these values as sampled/approximate in the response contract. 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%2F41611&comment_hash=2e5e41f6ffa5fb7b848a6d3ef7688251629728a47e483bd9523044bedf35808c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=2e5e41f6ffa5fb7b848a6d3ef7688251629728a47e483bd9523044bedf35808c&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]
