codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3632071112
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -642,3 +644,258 @@ def process(base_query_object: dict[str, Any]) ->
list[dict[str, Any]]:
return [result]
return build_query_context(self.data, process)
+
+
+def _get_table_chart_time_offsets(form_data: dict[str, Any]) -> list[Any]:
+ """
+ Resolve time_compare into the list of shifts buildQuery.ts sends as
+ time_offsets. table charts use a single-select time_compare control
+ whose choices include the special 'custom'/'inherit' shifts, which
+ resolve to start_date_offset/'inherit' rather than being used verbatim.
+ """
+ time_compare_shifts = ensure_is_array(form_data.get("time_compare"))
+ non_custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift not in ("custom",
"inherit")
+ ]
+ custom_or_inherit_shifts = [
+ shift for shift in time_compare_shifts if shift in ("custom",
"inherit")
+ ]
+
+ time_offsets: list[Any] = list(non_custom_or_inherit_shifts)
+ if "custom" in custom_or_inherit_shifts:
+ time_offsets.append(form_data.get("start_date_offset"))
+ if "inherit" in custom_or_inherit_shifts:
+ time_offsets.append("inherit")
+
+ # Dashboard filter override - allows dashboard-level time shifts to
+ # OVERRIDE chart-level time shift settings, mirroring buildQuery.ts.
+ extra_form_data_time_compare = (form_data.get("extra_form_data") or
{}).get(
+ "time_compare"
+ )
+ if extra_form_data_time_compare:
+ # extra_form_data.time_compare is typed as a single string on the
+ # frontend, but self.data comes from deserialized JSON with no
+ # runtime type guarantee — normalize defensively so an already-list
+ # value doesn't get double-nested into [[...]].
+ time_offsets = list(ensure_is_array(extra_form_data_time_compare))
+ return time_offsets
+
+
+def _reorder_table_chart_temporal_column(
+ columns: list[Any],
+ time_grain_sqla: Any,
+ temporal_columns_lookup: dict[str, Any],
+) -> list[Any]:
+ """
+ Move the first physical column with a temporal_columns_lookup entry to
+ the front of the columns list as a BASE_AXIS adhoc column, mirroring
+ buildQuery.ts's temporal-column handling in aggregate mode.
+ """
+ temporal_column = None
+ filtered_columns = []
+ for col in columns:
+ should_be_temporal = (
+ is_physical_column(col)
+ and time_grain_sqla
+ and temporal_columns_lookup.get(col)
+ )
+ if should_be_temporal and temporal_column is None:
+ temporal_column = {
+ "timeGrain": time_grain_sqla,
+ "columnType": "BASE_AXIS",
+ "sqlExpression": col,
+ "label": col,
+ "expressionType": "SQL",
+ }
+ else:
+ filtered_columns.append(col)
+ return [temporal_column] + filtered_columns if temporal_column else
filtered_columns
+
+
+class MigrateTableChart(MigrateViz):
+ source_viz_type = "table"
+ target_viz_type = "ag-grid-table"
+ remove_keys = {"allow_rearrange_columns", "allow_render_html"}
+ rename_keys: dict[str, str] = {} # no renames needed; names match 1:1
+
+ def _pre_action(self) -> None:
+ # page_length: 0 ("All") has no dropdown choice in v2, but the control
+ # is freeForm and 0 still works at runtime — map to v2's largest
+ # PAGE_SIZE_OPTIONS entry (200) so the migrated chart keeps showing as
+ # many rows per page as v2 supports, rather than an arbitrary smaller
+ # value
+ if self.data.get("page_length") in (0, "0"):
+ self.data["page_length"] = 200
+
+ # Table charts are explicitly excluded from Matrixify
+ # (MATRIXIFY_INCOMPATIBLE_CHARTS), so drop any matrixify_* keys
+ # rather than migrating them.
+ for key in [k for k in self.data if k.startswith("matrixify_")]:
+ self.data.pop(key)
+
+ def _build_aggregate_mode_query(
+ self, base_query_object: dict[str, Any], time_offsets: list[Any]
+ ) -> tuple[list[Any], list[Any], Any, list[Any]]:
+ """
+ Returns (metrics, columns, orderby, post_processing) for aggregate
+ mode, mirroring buildQuery.ts's QueryMode.Aggregate branch: sort-by
+ metric/default ordering, percent-metric contribution, time
+ comparison, and moving the temporal column to the front.
+ """
+ metrics = base_query_object.get("metrics") or []
+ orderby = base_query_object.get("orderby") or []
+ columns = list(base_query_object.get("columns") or [])
+ post_processing: list[Any] = []
+
+ sort_by_metric_options = ensure_is_array(
+ self.data.get("timeseries_limit_metric")
+ )
+ sort_by_metric = sort_by_metric_options[0] if sort_by_metric_options
else None
+ if sort_by_metric:
+ orderby = [[sort_by_metric, not self.data.get("order_desc",
False)]]
+ elif metrics:
+ orderby = [[metrics[0], False]]
+
+ if percent_metrics :=
ensure_is_array(self.data.get("percent_metrics")):
+ percent_metric_base_labels = [get_metric_label(m) for m in
percent_metrics]
+ if is_time_comparison(self.data, base_query_object):
+ # Mirror buildQuery.ts's addComparisonPercentMetrics: expand
+ # each percent metric with its time-offset suffixes so
+ # shifted percent columns are computed/renamed too.
+ percent_metric_labels_with_time_comparison = [
+ label
+ for metric_label in percent_metric_base_labels
+ for label in [
+ metric_label,
+ *[f"{metric_label}__{shift}" for shift in
time_offsets],
+ ]
+ ]
+ else:
+ percent_metric_labels_with_time_comparison =
percent_metric_base_labels
+ percent_metric_labels = remove_duplicates(
+ percent_metric_labels_with_time_comparison, get_metric_label
+ )
+ metrics = remove_duplicates(metrics + percent_metrics,
get_metric_label)
+ post_processing.append(
+ {
+ "operation": "contribution",
+ "options": {
+ "columns": percent_metric_labels,
+ "rename_columns": [f"%{m}" for m in
percent_metric_labels],
+ },
+ }
+ )
+
+ if time_offsets:
+ time_compare = time_compare_operator(self.data, base_query_object)
+ if time_compare:
+ post_processing.append(time_compare)
Review Comment:
**Suggestion:** The post-processing order is inverted when both time
comparison and percent metrics are enabled: `contribution` is appended before
`time_compare`, but contribution columns include time-shifted metric labels
that are only created by `time_compare`. This can make contribution computation
fail or produce incorrect results for comparison percent columns. Append
`time_compare` before `contribution` in this combined scenario. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Percent metrics incorrect in migrated AG Grid table charts.
- ⚠️ Affects charts using both percent and time compare.
- ⚠️ Undermines trust in migrated comparison tables.
- ⚠️ Breaks parity with Table V1 buildQuery semantics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or use an existing legacy Table V1 chart (`viz_type = "table"`)
that has both
percent metrics and time comparison configured via the table control panel
(`plugin-chart-table/src/controlPanel.tsx`), so its saved `params` JSON
contains
`percent_metrics` and a non-empty `time_compare` value along with other
table form data.
2. Run the `migrate_viz` CLI to migrate table charts to AG Grid V2. For each
`table`
Slice, the CLI ends up calling `MigrateViz.upgrade_slice()` in
`superset/migrations/shared/migrate_viz/base.py:150`, which instantiates
`MigrateTableChart` and eventually invokes `_build_query()` in
`superset/migrations/shared/migrate_viz/processors.py:861-901` to generate a
new
`query_context`.
3. Inside `_build_query()`, for aggregate mode the code calls
`_build_aggregate_mode_query()` at
`superset/migrations/shared/migrate_viz/processors.py:736-806`. When
`percent_metrics` is
set and `is_time_comparison(self.data, base_query_object)` is true, lines
759-777 build
`percent_metric_labels` including time-shifted labels such as `"metric__1
year ago"`, then
at lines 779-787 append a `"contribution"` post-processing step whose
`"columns"` option
references these labels. Immediately after, at lines 789-792, the
`"time_compare"`
post-processing step is appended.
4. When the migrated AG Grid table chart is executed, the backend runs the
`post_processing` steps in the order they were declared. Because
`"contribution"` is
placed before `"time_compare"`, it runs while only the base metric columns
exist;
time-shifted columns like `"metric__1 year ago"` are created later by
`"time_compare"`.
This mismatch means contribution either operates on missing columns or
produces incorrect
percent values for comparison metrics, so migrated table charts with both
percent metrics
and time comparison diverge from the intended V1 semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f0dde84a3154b32b7b694cebeece721&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=0f0dde84a3154b32b7b694cebeece721&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/migrations/shared/migrate_viz/processors.py
**Line:** 779:792
**Comment:**
*Logic Error: The post-processing order is inverted when both time
comparison and percent metrics are enabled: `contribution` is appended before
`time_compare`, but contribution columns include time-shifted metric labels
that are only created by `time_compare`. This can make contribution computation
fail or produce incorrect results for comparison percent columns. Append
`time_compare` before `contribution` in this combined scenario.
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%2F42088&comment_hash=afc29c61171c5133bac445a3e73a3a4617bdd201e8468bda433a1ca5a5cac47c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=afc29c61171c5133bac445a3e73a3a4617bdd201e8468bda433a1ca5a5cac47c&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]
