michael-s-molina commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3624010876
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -395,6 +395,25 @@ const buildQuery: BuildQuery<TableChartFormData> = (
const extraQueries: QueryObject[] = [];
+ const calculationMode = formData.percent_metric_calculation || 'row_limit';
+
+ if (
+ calculationMode === 'all_records' &&
+ percentMetrics &&
+ percentMetrics.length > 0
+ ) {
+ extraQueries.push({
+ ...queryObject,
+ columns: [],
+ metrics: percentMetrics,
+ post_processing: [],
+ row_limit: 0,
+ row_offset: 0,
+ orderby: [],
+ is_timeseries: false,
+ });
+ }
Review Comment:
Confirmed (also flagged by @rusackas above) and fixed in 4292578 — moved the
`all_records` extra-query construction to after the AG Grid/search filter
mutations, so it now builds from the fully-filtered `queryObject` instead of
the pre-filter one.
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -642,3 +644,225 @@ 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")
+ return time_offsets
Review Comment:
Confirmed and fixed in 4292578 — `_get_table_chart_time_offsets` now applies
the `extra_form_data.time_compare` override, mirroring buildQuery.ts's
precedence. Added a regression test
(`test_build_query_extra_form_data_time_compare_overrides_chart_level`).
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -642,3 +644,225 @@ 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")
+ 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_labels = remove_duplicates(
+ [get_metric_label(m) for m in percent_metrics],
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],
+ },
+ }
+ )
Review Comment:
Confirmed and fixed in 4292578 — percent-metric labels are now expanded with
time-offset suffixes when time comparison is enabled, mirroring
`addComparisonPercentMetrics`. Added a regression test
(`test_build_query_percent_metric_expands_with_time_comparison`).
##########
superset/migrations/shared/migrate_viz/processors.py:
##########
@@ -642,3 +644,225 @@ 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")
+ 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_labels = remove_duplicates(
+ [get_metric_label(m) for m in percent_metrics],
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)
+
+ columns = _reorder_table_chart_temporal_column(
+ columns,
+ self.data.get("time_grain_sqla"),
+ self.data.get("temporal_columns_lookup") or {},
+ )
Review Comment:
Confirmed and fixed in 4292578 — the effective `time_grain_sqla` used for
temporal-column promotion now respects the `extra_form_data` override,
mirroring buildQuery.ts (and the existing pivot-table processor's precedent).
Added a regression test
(`test_build_query_extra_form_data_time_grain_sqla_overrides_chart_level`).
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:
##########
@@ -439,6 +557,18 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
[setDataMask, serverPagination],
);
+ // Feeds the "Export Current View" menu item (EXPORT_CURRENT_VIEW behavior),
+ // mirroring Table V1's clientView snapshot on ownState.
+ const handleClientViewChange = useCallback(
+ (clientView: ClientViewSnapshot) => {
+ updateTableOwnState(setDataMask, {
+ ...serverPaginationData,
+ clientView,
+ });
+ },
Review Comment:
Confirmed and fixed in 4292578 — introduced a ref-backed `writeOwnState`
helper so all ownState writers (including `handleClientViewChange`, which can
fire from AG Grid's async `onModelUpdated`) merge onto the most recently
written state instead of a stale render-time snapshot.
--
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]
