codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r3577153011
##########
superset/common/query_context_processor.py:
##########
@@ -396,9 +431,310 @@ def get_payload(
},
self.get_cache_timeout(),
)
- return_value["cache_key"] = cache_key # type: ignore
+ return QueryContextExecutionResult(
+ queries=query_results,
+ cache_key=cache_key,
+ context_cache_write_outcome=context_cache_write_outcome,
+ )
- return return_value
+ def _execute_query_plan(
+ self,
+ force_cached: bool,
+ materialize: bool,
+ ) -> tuple[QueryDataResult, ...]:
+ """Execute one dependency-aware plan for every response mode."""
+
+ contribution_plan = self._contribution_plan()
+ requested_result_types = {
+ query_idx: get_effective_result_type(self._query_context, query)
+ for query_idx, query in enumerate(self._query_context.queries)
+ }
+ (
+ data_contribution_plan,
+ output_acquisitions,
+ contribution_totals,
+ ) = self._acquire_contribution_dependencies(
+ contribution_plan,
+ requested_result_types,
+ force_cached,
+ materialize,
+ )
+ query_results: dict[int, QueryDataResult] = {}
+
+ for query_idx, query_obj in enumerate(self._query_context.queries):
+ if query_idx in output_acquisitions:
+ continue
+ result_type = requested_result_types[query_idx]
+ if not is_data_result_type(result_type):
+ query_results[query_idx] = (
+ get_query_results_with_timing(
+ result_type,
+ self._query_context,
+ query_obj,
+ force_cached,
+ )
+ if materialize
+ else get_query_results_cache_only(
+ result_type,
+ self._query_context,
+ query_obj,
+ )
+ )
+ continue
+
+ totals_idx = data_contribution_plan.get(query_idx)
+ if totals_idx is not None and (
+ totals_idx not in contribution_totals
+ or not self._totals_support_consumer(
+ query_obj, contribution_totals.get(totals_idx, {})
+ )
+ ):
+ query_results[query_idx] = self._dependency_failed_result(
+ _("Contribution totals query failed")
+ )
+ continue
+ execution_query = (
+ self._with_contribution_totals(
+ query_obj, contribution_totals[totals_idx]
+ )
+ if totals_idx is not None
+ else query_obj
+ )
+ acquired = acquire_query_data(
+ result_type,
+ self._query_context,
+ execution_query,
+ force_cached if materialize else False,
+ detect_currency_value=materialize,
+ )
+ if acquired is not None:
+ output_acquisitions[query_idx] = acquired
+ continue
+ raise QueryObjectValidationError(
+ _("Data-backed result type did not produce a dataframe")
+ )
+
+ for query_idx, acquired in output_acquisitions.items():
+ query_results[query_idx] = (
+ materialize_acquired_query(self._query_context, acquired)
+ if materialize
+ else cache_acquired_query(acquired)
+ )
+ return tuple(
+ query_results[query_idx]
+ for query_idx in range(len(self._query_context.queries))
+ )
+
+ def _acquire_contribution_dependencies(
+ self,
+ contribution_plan: dict[int, int],
+ requested_result_types: dict[int, ChartDataResultType],
+ force_cached: bool,
+ materialize: bool,
+ ) -> tuple[
+ dict[int, int],
+ dict[int, AcquiredQuery],
+ dict[int, dict[str, Any]],
+ ]:
+ """Acquire dataframe producers needed by data-backed consumers."""
+
+ data_plan = {
+ consumer_idx: producer_idx
+ for consumer_idx, producer_idx in contribution_plan.items()
+ if is_data_result_type(requested_result_types[consumer_idx])
+ }
+ output_acquisitions: dict[int, AcquiredQuery] = {}
+ totals: dict[int, dict[str, Any]] = {}
+ for producer_idx in sorted(set(data_plan.values())):
+ acquired = acquire_query_data(
+ ChartDataResultType.FULL,
+ self._query_context,
+ self._totals_query(producer_idx),
+ force_cached if materialize else False,
+ detect_currency_value=materialize,
+ )
+ if acquired is None:
+ raise QueryObjectValidationError(
+ _("Contribution totals require a dataframe result type")
+ )
+ if is_data_result_type(requested_result_types[producer_idx]):
+ output_acquisitions[producer_idx] = acquired
Review Comment:
**Suggestion:** The dependency prefetch path stores the totals-query
acquisition as the producer query’s final output. Because `_totals_query()`
mutates the producer query (at least `row_limit`), the API can return data for
a different query than the one originally requested at that index. Keep the
totals acquisition only for contribution injection, and acquire/materialize the
producer’s own query object separately for its response entry. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Contribution charts reuse normalized totals queries as responses.
- ⚠️ Chart data API returns data for mutated totals queries.
- ⚠️ MCP semantic tools see totals differing from configured queries.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a QueryContext with two queries as in
`test_contribution_totals_are_reused_without_mutating_queries`
(superset/tests/unit_tests/common/test_query_context_processor_timing.py:70-84):
a main
query with `columns=["region"]`, `metrics=["sales"]`, `row_limit=100` and a
totals query
with `columns=[]`, `metrics=["sales"]`, `row_limit=100`, and set
`query_context.result_type = ChartDataResultType.FULL`.
2. Instantiate `QueryContextProcessor` with this QueryContext and call
`processor._execute_query_plan(force_cached=False, materialize=True)`
(superset/common/query_context_processor.py:440-526), which internally calls
`_acquire_contribution_dependencies()` (lines 528-566) to prefetch
contribution
dependencies.
3. Inside `_acquire_contribution_dependencies`, observe that for
`producer_idx=1` it
executes `acquired = acquire_query_data(ChartDataResultType.FULL,
self._query_context,
self._totals_query(producer_idx), ...)` (lines 549-555); `_totals_query()`
(lines 692-695)
clones the producer query and sets `row_limit = None`, then because
`requested_result_types[producer_idx]` is a data result type, stores this
mutated
acquisition in `output_acquisitions[producer_idx]` (lines 560-561).
4. Back in `_execute_query_plan`, the loop skips index `1` entirely because
it is present
in `output_acquisitions` (lines 464-466), and later materializes that stored
acquisition
via `materialize_acquired_query(self._query_context, acquired)` (lines
517-523), so the
`QueryDataResult` at index `1`—and the corresponding entry returned by
`ChartDataCommand.execute()` in
superset/commands/chart/data/get_data_command.py:74-85—reflects the cloned
totals query
(with `row_limit=None`) instead of the original user-configured
`query_context.queries[1]`
object.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=94b82e3952f44ae09fbd60a43d95ae92&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=94b82e3952f44ae09fbd60a43d95ae92&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/common/query_context_processor.py
**Line:** 549:561
**Comment:**
*Logic Error: The dependency prefetch path stores the totals-query
acquisition as the producer query’s final output. Because `_totals_query()`
mutates the producer query (at least `row_limit`), the API can return data for
a different query than the one originally requested at that index. Keep the
totals acquisition only for contribution injection, and acquire/materialize the
producer’s own query object separately for its response entry.
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%2F37516&comment_hash=35c43c6211ea92cce9e21d3ee22eeac95ef225713f79dd21730df87b43a3fbc6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=35c43c6211ea92cce9e21d3ee22eeac95ef225713f79dd21730df87b43a3fbc6&reaction=dislike'>👎</a>
##########
superset/common/query_context_processor.py:
##########
@@ -438,37 +774,39 @@ def get_annotation_data(self, query_obj: QueryObject) ->
dict[str, Any]:
@staticmethod
def get_native_annotation_data(query_obj: QueryObject) -> dict[str, Any]:
- annotation_data = {}
- annotation_layers = [
- layer
- for layer in query_obj.annotation_layers
- if layer["sourceType"] == "NATIVE"
- ]
- layer_ids = [layer["value"] for layer in annotation_layers]
- layer_objects = {
- layer_object.id: layer_object
- for layer_object in AnnotationLayerDAO.find_by_ids(layer_ids)
- }
+ with source_phase("planning"):
+ annotation_layers = [
+ layer
+ for layer in query_obj.annotation_layers
+ if layer["sourceType"] == "NATIVE"
+ ]
+ layer_ids = [layer["value"] for layer in annotation_layers]
+
+ with source_phase("execution"):
+ layer_objects = {
+ layer_object.id: list(layer_object.annotation)
+ for layer_object in AnnotationLayerDAO.find_by_ids(layer_ids)
+ }
- # annotations
- for layer in annotation_layers:
- layer_id = layer["value"]
- layer_name = layer["name"]
+ with source_phase("processing"):
+ annotation_data = {}
columns = [
"start_dttm",
"end_dttm",
"short_descr",
"long_descr",
"json_metadata",
]
- layer_object = layer_objects[layer_id]
- records = [
- {column: getattr(annotation, column) for column in columns}
- for annotation in layer_object.annotation
- ]
- result = {"columns": columns, "records": records}
- annotation_data[layer_name] = result
- return annotation_data
+ for layer in annotation_layers:
+ records = [
+ {column: getattr(annotation, column) for column in columns}
+ for annotation in layer_objects[layer["value"]]
Review Comment:
**Suggestion:** This directly indexes `layer_objects` by layer ID and will
raise `KeyError` if an annotation layer ID is missing (for example, deleted or
inaccessible between save and execution), causing a 500 instead of a controlled
validation error. Use a safe lookup and raise `QueryObjectValidationError` (or
skip missing layers explicitly) when a referenced layer is absent. [possible
bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Charts with missing native annotations crash with 500 errors.
- ⚠️ Users lose chart diagnostics when annotation layers are deleted.
- ⚠️ Tools consuming chart data fail on missing annotation layers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart whose query object includes a native annotation layer
with
`sourceType: "NATIVE"` and a `value` pointing to an annotation layer ID that
has been
deleted or is no longer visible, so `query_obj.annotation_layers` contains
such an entry;
this is the structure consumed by
`QueryContextProcessor.get_annotation_data()`
(superset/common/query_context_processor.py:762-773).
2. Call the chart data API (e.g., `/api/v1/chart/data`) which constructs a
`QueryContext`
and executes `ChartDataCommand.run()`
(superset/commands/chart/data/get_data_command.py:66-72); `run()` calls
`execute()`, which
invokes `query_context.get_payload_result()`
(superset/common/query_context.py:27-38),
delegating to `QueryContextProcessor.get_payload_result()`
(superset/common/query_context_processor.py:408-438).
3. During query execution, `get_df_payload_result()`
(superset/common/query_context_processor.py:121-308) processes annotation
layers and calls
`self.get_annotation_data(query_obj)` (lines 193-195), which in turn invokes
the static
`get_native_annotation_data()` to fetch native annotation records
(superset/common/query_context_processor.py:775-809).
4. In `get_native_annotation_data()`,
`AnnotationLayerDAO.find_by_ids(layer_ids)`
(superset/daos/base.py:24-32, 47-65) may return fewer layer objects than IDs
requested
when some layers are missing or filtered, so `layer_objects` lacks keys for
some
`layer["value"]`; when the code loops `for layer in annotation_layers:` and
indexes
`layer_objects[layer["value"]]` directly
(superset/common/query_context_processor.py:800-803), a missing ID triggers
a `KeyError`
rather than `QueryObjectValidationError`, resulting in an unhandled
exception and a 500
error for the chart data request.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=03526e041720446a8e3db9ca8f7dafb2&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=03526e041720446a8e3db9ca8f7dafb2&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/common/query_context_processor.py
**Line:** 800:803
**Comment:**
*Possible Bug: This directly indexes `layer_objects` by layer ID and
will raise `KeyError` if an annotation layer ID is missing (for example,
deleted or inaccessible between save and execution), causing a 500 instead of a
controlled validation error. Use a safe lookup and raise
`QueryObjectValidationError` (or skip missing layers explicitly) when a
referenced layer is absent.
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%2F37516&comment_hash=f855fba11c6e44c48f617a5db8cf27ecf9572b0fec5bb2aa38cc4047555e8fc3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37516&comment_hash=f855fba11c6e44c48f617a5db8cf27ecf9572b0fec5bb2aa38cc4047555e8fc3&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]
