EnxDev commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3968940039
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -654,3 +656,268 @@ 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], base_query_object: 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.
+
+ Chart-level shifts only apply when is_time_comparison(...) holds,
+ mirroring buildQuery.ts; the dashboard-level extra_form_data override
+ below is applied regardless, since it can force a comparison the chart
+ itself isn't configured for.
+ """
+ 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] = []
+ if is_time_comparison(form_data, base_query_object):
+ time_offsets = 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"
+ # allow_rearrange_columns/allow_render_html are kept as-is: v2 reads them
+ # under the same names (see rename_keys below), so nothing to remove.
+ remove_keys: set[str] = set()
+ rename_keys: dict[str, str] = {} # no renames needed; names match 1:1
+
+ def _pre_action(self) -> None:
+ # page_length: 0 means "All rows" (no pagination) in both v1 and v2.
+ # v2's control panel doesn't offer 0 as a page_length dropdown
+ # choice, but it's still a working runtime value there -- e.g.
+ # getPageSize() in transformProps.ts picks 0 automatically for any
+ # chart under 5000 cells when page_length isn't set at all -- so
+ # keep it as-is rather than rewriting it to a paginated value.
+
+ # 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)
+
+ # Dashboard-level grain override takes precedence over the
+ # chart-level time_grain_sqla, mirroring buildQuery.ts.
+ extra_form_data_time_grain = (self.data.get("extra_form_data") or
{}).get(
+ "time_grain_sqla"
+ )
+ time_grain_sqla = extra_form_data_time_grain or
self.data.get("time_grain_sqla")
+ columns = _reorder_table_chart_temporal_column(
+ columns,
+ time_grain_sqla,
+ self.data.get("temporal_columns_lookup") or {},
+ )
+
+ return metrics, columns, orderby, post_processing
+
+ def _build_table_chart_extra_queries(
+ self, query_object: dict[str, Any]
+ ) -> list[dict[str, Any]]:
+ """
+ Extra queries appended after the main query: an unlimited
+ percent-metrics-only query for percent_metric_calculation ==
+ 'all_records', and a totals query when show_totals is on.
+ """
+ percent_metrics = ensure_is_array(self.data.get("percent_metrics"))
+ calculation_mode = self.data.get("percent_metric_calculation") or
"row_limit"
+ metrics = query_object.get("metrics")
+ contribution_post_processing = next(
+ (
+ pp
+ for pp in query_object.get("post_processing") or []
+ if pp.get("operation") == "contribution"
+ ),
+ None,
+ )
+
+ extra_queries = []
+
+ if calculation_mode == "all_records" and percent_metrics:
+ extra_queries.append(
+ {
+ **query_object,
+ "columns": [],
+ "metrics": percent_metrics,
+ "post_processing": [],
+ "row_limit": 0,
+ "row_offset": 0,
+ "orderby": [],
+ "is_timeseries": False,
+ }
+ )
+
+ if metrics and self.data.get("show_totals"):
+ extra_queries.append(
+ {
+ **omit(query_object, ["order_desc", "orderby"]),
Review Comment:
**[P1] Apply `totals_aggregate` when rebuilding the totals query.** Both
runtime `buildQuery.ts` paths call `getTotalsMetrics(metrics,
toTotalsAggregate(formData.totals_aggregate))`, but the migration reuses the
main query’s metrics unchanged here. A chart saved with `show_totals: true`,
`totals_aggregate: 'AVG'`, and a SIMPLE `SUM(...)` metric therefore gets a
persisted totals query that still computes `SUM`. Could we mirror that metric
conversion here and cover at least the `SUM` → `AVG` case in
`table_v1_v2_test.py`?
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:
##########
@@ -526,10 +588,10 @@ const transformProps = (
comparison_color_enabled: comparisonColorEnabled = false,
comparison_color_scheme: comparisonColorScheme = ColorSchemeEnum.Green,
show_numbered_column: showNumberedColumn = false,
+ allow_rearrange_columns: allowRearrangeColumns = true,
Review Comment:
**[P2] Preserve the V1 default when this flag is absent.** V1’s control and
`TableChart` both default `allow_rearrange_columns` to `false`, and older saved
charts may not contain the key at all. After migration, those charts hit this
`true` fallback, so columns become draggable even though the equivalent V1
chart was not (and the new V2 control also defaults to `false`). Could we
default this to `false`, or materialize the V1 default in `MigrateTableChart`,
and add a migration case where the key is omitted?
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -400,11 +406,26 @@ const ChartContextMenu = (
filters,
});
- // Since Ant Design's Dropdown does not offer an imperative API
- // and we can't attach event triggers to charts SVG elements, we
- // use a hidden span that gets clicked on when receiving click events
- // from the charts.
- document.getElementById(`hidden-span-${id}`)?.click();
+ // Some chart libraries (e.g. AG Grid) can dispatch a single logical
+ // right-click as two contextmenu events in quick succession, calling
+ // `open()` twice. Since Ant Design's Dropdown treats a click on an
+ // already-open trigger as a toggle-to-close, re-clicking the hidden
+ // span here on the second call would immediately close the menu we
+ // just opened. Only click it when the menu isn't already visible; the
+ // position/filters update above still applies on every call.
+ //
+ // visibleRef (not the `visible` state) drives this guard: the state
+ // update from the first call's click hasn't been committed by the time
+ // the second call runs, so a state-based check would still read the
+ // stale `false` from this render's closure and click twice anyway.
+ if (!visibleRef.current) {
Review Comment:
**[P1] Reset the ref when Drill By closes the menu.** This guard reads
`visibleRef`, but `closeContextMenu` only calls `setVisible(false)`.
`DrillBySubmenu` invokes that callback after a column is selected, and closing
the controlled Dropdown that way does not guarantee another
`onOpenChange(false)` callback, so the ref can remain `true` and the next
`open()` skips the trigger click. Could we also set `visibleRef.current =
false` inside `closeContextMenu` and cover open → Drill By selection → open
again?
--
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]
