You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/10 13:33:05 UTC
[GitHub] [superset] zhaoyongjie opened a new pull request, #20010: feat: standardized form_data
zhaoyongjie opened a new pull request, #20010:
URL: https://github.com/apache/superset/pull/20010
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
TBD
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1125014839
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1127316944
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873734921
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
Agreed, `percent_metrics` always makes me cringe for that exact reason (=it's really a post transform op). I'm fine leaving it out for now, as we can easily iteratively improve this functionality as we don't have to worry about backward compatibility.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1129522205
> @zhaoyongjie Is 2 about allowing users to restore field values when they switch between chart types?
Yes. The `memorizedFormData`(field in StandardizeFormData) saved the previous chart type as a key, and form_data as a value. In order to find the latest chart, the array was used to store it.
> Would it suffice if we just keep the history in a reducer state as well? It could be a simple vizType to formData dict. A plugin's fromStandardizedState function will accept both the standardized state from current formData and the previous formData stored and decide how to merge them.
I've tried this, and as you can see, it's hard to read the logic of the current [reducer](https://github.com/apache/superset/blob/63702c48ab77ee73b7e304c92fc74ce02748107e/superset-frontend/src/explore/reducers/exploreReducer.js#L132).
In abstract terms. We just need to collect/calculate the current `form_data` and `controlsState` for "arrived viz", and I've written the steps on how to do that in the [comments](https://github.com/apache/superset/pull/20010/files#diff-22f8206e02d866fc191fba5d77a5578c8a1008926e949a983745795e614aa7d4R142-R150). IMO, we should "encapsulate" the transformation logic rather than “leak” information.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873767947
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
>This way the pie chart might not care if the metric was from metrics or percent_metrics, but some other chart might decide to map the values over differently, depending on where the value came from. This is especially true for the x_axis control, which in a sense is a column/groupby, but should be handled differently from the series control.
The `xaxis` didn't put in the `sharedControls`. it's a publicControls. In the other words, the x-axis just "inhert" latest chart x-axis value, it can not mapping from `sharedControls`.
I suggest that the modeling(like metric/dimension/filter/etc...) should decouple from visualization. The `sourceControl: string;` means that preserve the controls context, this will allow modeling and visualization to be mixed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872367864
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ sharedFormData[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardized_form_data?.memorizedFormData,
+ )
+ ? new Map(formData.standardized_form_data.memorizedFormData)
+ : new Map();
+ const vizType = formData.viz_type;
+ if (memorizedFormData.has(vizType)) {
+ memorizedFormData.delete(vizType);
+ }
+ memorizedFormData.set(vizType, formData);
+ this.sfd = {
+ sharedFormData,
+ memorizedFormData,
+ };
+ }
+
+ static getReversedMap() {
+ const reversedMap = new Map();
+ Object.entries(sharedControls).forEach(([key, names]) => {
+ names.forEach(name => {
+ reversedMap.set(name, key);
+ });
+ });
+ return reversedMap;
+ }
+
+ private getLatestFormData(vizType: string): QueryFormData {
+ if (this.sfd.memorizedFormData.has(vizType)) {
+ return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
+ }
+
+ return this.memorizedFormData.slice(-1)[0][1];
Review Comment:
the `memorizedFormData` will not be empty, it must be initialized in the constructor.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873730892
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
> I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
This is an interesting question. It is also a legacy of Superset's history. The `Percentage Metrics` is a post-processing **metric** rather than a SQL base **metric**. basically, it is not simply mapping from a source viz to a target viz. The same control is `Contribution Mode` in the Line chart V2.
I have said before, to solve this issue, I think we eventually need to move all post-processing(AA) into the metric and column so that we can do the real map between different charts.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873586913
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
Thanks for the response - I must have had something wrong with my devenv, as after I reloaded it worked just like you described 👍 To add to this comment:
> So I think that when we transition from Pivot Table to any other chart we should join values from Rows and Columns, remove duplicates and populate Dimensions of the new viz with the result
I think we should keep the duplicates in the normalized form data, but we could enrich the interface so that we know where the values came from:
```typescript
export interface SharedFormData {
metrics: {
value: QueryFormMetric;
sourceViz: string;
sourceControl: string;
}[];
columns: {
value: QueryFormColumn;
sourceViz: string;
sourceControl: string;
}[];
}
```
This way the pie chart might not care if the metric was from `metrics` or `percent_metrics`, but some other chart might decide to map the values over differently, depending on where the value came from. This is especially true for the `x_axis` control, which in a sense is a column/groupby, but should be handled differently from the series control.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873704415
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
Review Comment:
Yes, the `sharedformData` is not an appropriate name here. Maybe `SharedControlls` / `StandardizedState` is more direct. I am also considering whether should we rename the `StandardizedFormData`(the class name) -> `StandardizedFormDataState` or `IntermediateFormData`. How about you think?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873767947
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
>This way the pie chart might not care if the metric was from metrics or percent_metrics, but some other chart might decide to map the values over differently, depending on where the value came from. This is especially true for the x_axis control, which in a sense is a column/groupby, but should be handled differently from the series control.
The `xaxis` didn't put in the `sharedControls`. it's a publicControls. In the other words, the x-axis just "inhert" latest chart x-axis value, it can not mapping from `sharedControls`.
I suggest that the modeling(like metric/dimension/filter/etc...) should decouple from visualization. The `sourceControl: string;` means that preserve the control context, this will allow modeling and visualization to be mixed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1128205078
Have to take a deeper look at this, but I'm not sure we need another layer of transformation for control states... what if we just run db migrations for all existing viz types to change some non-standard control fields to standardized names?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1146801190
Ephemeral environment shutdown and build artifacts deleted.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r874211094
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +342,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface standardizedState {
Review Comment:
```suggestion
export interface StandardizedState {
```
Types should use `PascalCase`.
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ standardizedState,
+ StandardizedFormDataInterface,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof standardizedState, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: StandardizedFormDataInterface;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const standardizedState = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = Object.freeze(sourceFormData);
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ standardizedState[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardizedorm_data?.memorizedFormData,
Review Comment:
```suggestion
formData?.standardized_form_data?.memorizedFormData,
```
Looks like there's a typo here
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r874280615
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ standardizedState,
+ StandardizedFormDataInterface,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof standardizedState, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: StandardizedFormDataInterface;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const standardizedState = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = Object.freeze(sourceFormData);
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ standardizedState[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardizedorm_data?.memorizedFormData,
Review Comment:
I have added a unit test for the memorizedFormData.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873684631
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
+}
+
+export interface StandardizedFormDataInterface {
Review Comment:
Yep... the `StandardizedFormData` has been class name.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873704415
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
Review Comment:
Yes, the `sharedformData` is not an appropriate name here. Maybe `SharedControlls` / `StandardizedState` is more direct. I am also considering whether should we rename the `StandardizedFormData` -> `StandardizedFormDataState` or `IntermediateFormData`. How about you think?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873730892
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
> I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
This is an interesting question. It is also a legacy of Superset's history. The `Percentage Metrics` is a post-processing **metric** rather than a **SQL base metric**. basically, it can not simply be mapping from a source viz to a target viz. The same control is `Contribution Mode` in the Line chart V2.
I have said before, to solve this issue, I think we eventually need to move all post-processing(AA) into the metric and column so that we can do the real mapping between different charts.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1126015156
> Found a bug while switching between pivot table and and other charts. When pivot table has the same value added in Columns and Rows controls, then that value is going to be duplicated when switched to a chart with columns control.
> Screen.Recording.2022-05-13.at.13.38.00.mov
Thanks, Kamil. I also noticed this but I want to discuss it with you for this. Since the Pivot Table allows `Row` and `Columns` to have the same columns, I didn't deduplicate the name of columns when collecting sharedControl. The current solution also provides the same view between Table and PivotTable. So, I think that if we need to add the restriction on the SharedControl, we also need to add this in Pivot Table.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873491796
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
As a solution - Maybe we could add a property to the normalized control values that states some additional context about the origin of the value. In this case it could specify that it originated from the `percent_metrics` control rather than a "regular" metric control. So the additional value could, for instance, be `sourceControl`.
This would also address the problem identified in the Pivot table - if we had the additional context about what type of a control the value originated from, we would more easily be able to map them back to their original controls (`groupbyRows` or `groupbyColums`). Ping @kgabryje
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
As a solution - Maybe we could add a property to the normalized control values that states some additional context about the origin of the value. In this case it could specify that it originated from the `percent_metrics` control rather than a "regular" metric control. So the additional value could, for instance, be called `sourceControl`.
This would also address the problem identified in the Pivot table - if we had the additional context about what type of a control the value originated from, we would more easily be able to map them back to their original controls (`groupbyRows` or `groupbyColums`). Ping @kgabryje
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r874259663
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ standardizedState,
+ StandardizedFormDataInterface,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof standardizedState, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: StandardizedFormDataInterface;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const standardizedState = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = Object.freeze(sourceFormData);
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ standardizedState[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardizedorm_data?.memorizedFormData,
Review Comment:
Thanks! it a typo.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872302976
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,24 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
+}
+
+export interface iStandardizedFormData {
Review Comment:
We don't use "i" prefix for interface anywhere else in the codebase. Maybe we should rename it and stick to PascalCase?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1125968336
Found a bug while switching between pivot table and and other charts. When pivot table has the same value added in Columns and Rows controls, then that value is going to be duplicated when switched to a chart with columns control.
https://user-images.githubusercontent.com/15073128/168275708-a9aa0bcd-2396-497e-979c-ebe1446715d3.mov
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1124469767
# [Codecov](https://codecov.io/gh/apache/superset/pull/20010?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20010](https://codecov.io/gh/apache/superset/pull/20010?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b8bd5ce) into [master](https://codecov.io/gh/apache/superset/commit/60188ef65476c534647db813c35add3236076cec?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60188ef) will **decrease** coverage by `0.01%`.
> The diff coverage is `42.85%`.
```diff
@@ Coverage Diff @@
## master #20010 +/- ##
==========================================
- Coverage 66.35% 66.33% -0.02%
==========================================
Files 1712 1714 +2
Lines 64060 64106 +46
Branches 6742 6748 +6
==========================================
+ Hits 42508 42526 +18
- Misses 19841 19867 +26
- Partials 1711 1713 +2
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.29% <42.85%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20010?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...harts/src/BigNumber/BigNumberTotal/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlclRvdGFsL2NvbnRyb2xQYW5lbC50cw==) | `0.00% <0.00%> (ø)` | |
| [.../BigNumber/BigNumberWithTrendline/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvY29udHJvbFBhbmVsLnRzeA==) | `25.00% <0.00%> (-8.34%)` | :arrow_down: |
| [...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvUGllL2NvbnRyb2xQYW5lbC50c3g=) | `33.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...rt-echarts/src/Timeseries/Regular/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy9SZWd1bGFyL2NvbnRyb2xQYW5lbC50c3g=) | `33.33% <0.00%> (-6.67%)` | :arrow_down: |
| [...d/src/explore/controlUtils/standardizedFormData.ts](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzL3N0YW5kYXJkaXplZEZvcm1EYXRhLnRz) | `45.94% <45.94%> (ø)` | |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/superset/pull/20010/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `28.23% <50.00%> (-0.52%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20010?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20010?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [60188ef...b8bd5ce](https://codecov.io/gh/apache/superset/pull/20010?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873478705
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
+}
+
+export interface StandardizedFormDataInterface {
Review Comment:
Do we need the `Interface` suffix here?
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
Review Comment:
You can also use this:
```suggestion
columns: QueryFormColumn[];
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/controlPanel.ts:
##########
@@ -96,4 +96,8 @@ export default {
label: t('Number format'),
},
},
+ denormalizeFormData: formData => ({
+ ...formData,
+ metric: formData.standardized_form_data.sharedFormData.metrics[0],
Review Comment:
I know this mixing of camel and snake case is rampant from before, but I wonder if we could just call it `standardizedFormData` to make our eyes hurt slightly less? 😆
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
As a solution - Maybe we could add a property to the normalized control values that states some additional context about the origin of the value. In this case it could specify that it originated from the `percent_metrics` control rather than a "regular" metric control. So the additional value coult, for instance, be `sourceControl`.
This would also address the problem identified in the Pivot table - if we had the additional context about what type of a control the value originated from, we would more easily be able to map them back to their original controls (`groupbyRows` or `groupbyColums`). Ping @kgabryje
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
Review Comment:
Would it make sense to call this `NormalizedFormData` or similar? "Shared" feels slightly too generic, and "normalized" or "standardized" would more clearly indicate that a transformation has taken place. To disambiguate with the already existing `StandardizedFormDataInterface`, maybe we should consider also renaming that to `StandardizedState`, as it's not directly `FormData`, but a kind-of struct containing "generic stuff" that's needed to perform the normalization operations.
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
Review Comment:
Shouldn't this be `QueryFormMetric` to also handle saved metrics?
```suggestion
metrics: QueryFormMetric[];
```
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.test.tsx:
##########
@@ -0,0 +1,267 @@
+/**
+ * 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.
+ */
+import { getChartControlPanelRegistry, SqlaFormData } from '@superset-ui/core';
+import TableChartPlugin from '@superset-ui/plugin-chart-table';
+import { BigNumberTotalChartPlugin } from '@superset-ui/plugin-chart-echarts';
+import { sections } from '@superset-ui/chart-controls';
+import {
+ StandardizedFormData,
+ sharedControls,
+ publicControls,
+} from './standardizedFormData';
+import { xAxisControl } from '../../../plugins/plugin-chart-echarts/src/controls';
+
+describe('should collect control values and create SFD', () => {
+ const sharedControlsFormData = {};
+ Object.entries(sharedControls).forEach(([, names]) => {
+ names.forEach(name => {
+ sharedControlsFormData[name] = name;
+ });
+ });
+ const publicControlsFormData = Object.fromEntries(
+ publicControls.map((name, idx) => [[name], idx]),
+ );
+ const sourceMockFormData: SqlaFormData = {
+ ...sharedControlsFormData,
+ ...publicControlsFormData,
+ datasource: '100__table',
+ viz_type: 'source_viz',
+ };
+ const sourceMockStore = {
+ form_data: sourceMockFormData,
+ controls: Object.fromEntries(
+ Object.entries(sourceMockFormData).map(([key, value]) => [
+ key,
+ { value },
+ ]),
+ ),
+ datasource: {
+ type: 'table',
+ columns: [],
+ },
+ };
+ beforeAll(() => {
+ getChartControlPanelRegistry().registerValue('source_viz', {
+ controlPanelSections: [
+ sections.advancedAnalyticsControls,
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls.map(control => [control]),
+ },
+ {
+ label: 'axis column',
+ controlSetRows: [xAxisControl],
+ },
+ ],
+ });
+ getChartControlPanelRegistry().registerValue('target_viz', {
+ controlPanelSections: [
+ sections.advancedAnalyticsControls,
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls.map(control => [control]),
+ },
+ {
+ label: 'axis column',
+ controlSetRows: [[xAxisControl]],
+ },
+ ],
+ denormalizeFormData: (formData: SqlaFormData) => ({
+ ...formData,
+ columns: formData.standardized_form_data.sharedFormData.columns,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
+ }),
+ });
+ });
+
+ test('collect sharedControls', () => {
+ const sfd = new StandardizedFormData(sourceMockFormData);
+
+ expect(sfd.dumpSFD().sharedFormData.metrics).toEqual(
+ sharedControls.metrics.map(controlName => controlName),
+ );
+ expect(sfd.dumpSFD().sharedFormData.columns).toEqual(
+ sharedControls.columns.map(controlName => controlName),
+ );
+ });
+
+ test('should transform all publicControls', () => {
+ const sfd = new StandardizedFormData(sourceMockFormData);
+ const { formData } = sfd.transform('target_viz', sourceMockStore);
+ Object.entries(publicControlsFormData).forEach(([key]) => {
+ expect(formData).toHaveProperty(key);
+ });
+ Object.entries(sharedControls).forEach(([key, value]) => {
+ expect(formData[key]).toEqual(value);
+ });
+ });
+});
+
+describe('should transform form_data between table and bigNumberTotal', () => {
+ const tableVizFormData = {
+ datasource: '30__table',
+ viz_type: 'table',
+ time_grain_sqla: 'P1D',
+ time_range: 'No filter',
+ query_mode: 'aggregate',
+ groupby: ['name'],
+ metrics: ['count'],
+ all_columns: [],
+ percent_metrics: [],
+ adhoc_filters: [],
+ order_by_cols: [],
+ row_limit: 10000,
+ server_page_length: 10,
+ order_desc: true,
+ table_timestamp_format: 'smart_date',
+ show_cell_bars: true,
+ color_pn: true,
+ applied_time_extras: {},
+ url_params: {
+ form_data_key:
+ 'p3No_sqDW7k-kMTzlBPAPd9vwp1IXTf6stbyzjlrPPa0ninvdYUUiMC6F1iKit3Y',
+ dataset_id: '30',
+ },
+ };
+ const tableVizStore = {
+ form_data: tableVizFormData,
+ controls: {
+ datasource: {
+ value: '30__table',
+ },
+ viz_type: {
+ value: 'table',
+ },
+ slice_id: {},
+ cache_timeout: {},
+ url_params: {
+ value: {
+ form_data_key:
+ 'p3No_sqDW7k-kMTzlBPAPd9vwp1IXTf6stbyzjlrPPa0ninvdYUUiMC6F1iKit3Y',
+ dataset_id: '30',
+ },
+ },
+ granularity_sqla: {},
+ time_grain_sqla: {
+ value: 'P1D',
+ },
+ time_range: {
+ value: 'No filter',
+ },
+ query_mode: {
+ value: 'aggregate',
+ },
+ groupby: {
+ value: ['name'],
+ },
+ metrics: {
+ value: ['count'],
+ },
+ all_columns: {
+ value: [],
+ },
+ percent_metrics: {
+ value: [],
+ },
+ adhoc_filters: {
+ value: [],
+ },
+ timeseries_limit_metric: {},
+ order_by_cols: {
+ value: [],
+ },
+ server_pagination: {},
+ row_limit: {
+ value: 10000,
+ },
+ server_page_length: {
+ value: 10,
+ },
+ include_time: {},
+ order_desc: {
+ value: true,
+ },
+ show_totals: {},
+ emit_filter: {},
+ table_timestamp_format: {
+ value: 'smart_date',
+ },
+ page_length: {},
+ include_search: {},
+ show_cell_bars: {
+ value: true,
+ },
+ align_pn: {},
+ color_pn: {
+ value: true,
+ },
+ column_config: {},
+ conditional_formatting: {},
+ },
+ datasource: {
+ type: 'table',
+ columns: [],
+ },
+ };
+
+ beforeAll(() => {
+ getChartControlPanelRegistry().registerValue(
+ 'big_number_total',
+ new BigNumberTotalChartPlugin().controlPanel,
+ );
+ getChartControlPanelRegistry().registerValue(
+ 'table',
+ new TableChartPlugin().controlPanel,
+ );
+ });
+
+ test('transform', () => {
+ // table -> bigNumberTotal
+ const sfd = new StandardizedFormData(tableVizFormData);
+ const { formData: bntFormData, controlsState: btnControlsState } =
Review Comment:
I assume this is a typo:
```suggestion
const { formData: bntFormData, controlsState: bntControlsState } =
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873730538
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
The good thing here is this is all ephemeral state, so we don't have to be super cautious about breaking changes. So I vote iterating in small increments and not over-abstracting right now (which is precisely what I was doing 😆 )
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873767947
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
>This way the pie chart might not care if the metric was from metrics or percent_metrics, but some other chart might decide to map the values over differently, depending on where the value came from. This is especially true for the x_axis control, which in a sense is a column/groupby, but should be handled differently from the series control.
The `xaxis` didn't put in the `sharedControls`. it's a publicControls. In the other words, the x-axis just "inhert" latest chart x-axis value, it can not mapping from `sharedControls`.
I suggest that the modeling(like metric/dimension/filter/etc...) should decouple from visualization. The `sourceControl: string;` means that preserving the context of the control, will allow modeling and visualization to be mixed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1128320199
> I'm not sure we need to store the standardized form data in a state object. The re-mapping calculation should be ephemeral and wouldn't be useful once users have done switching viz types.
The Standardized Form Data has 2 uses.
1. re-mapping the value of controls
2. save switching history and retrieve the latest form_data by the viz type.
The first use case can easily achieve by defining from/to function in each plugin, but the second use case has to save in form_data.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872374698
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
Review Comment:
Good catch. I have moved the shallow copy to the reducer. I would really like to have a freeze object data structure here😆.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1125017253
@zhaoyongjie Ephemeral environment spinning up at http://52.34.220.70:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872359441
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,24 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
+ metrics: AdhocMetric[];
+ columns: (AdhocColumn | PhysicalColumn)[];
+}
+
+export interface iStandardizedFormData {
Review Comment:
Good catch. the `i` is for the `interface` from some other language styling. I will update it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1127321510
@stephenLYZ Ephemeral environment spinning up at http://34.217.96.104:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873713319
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
> I think we should keep the duplicates in the normalized form data
I'm still not really sold on this, because we can enter a state which is not allowed via user interface
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1128322997
@zhaoyongjie Ephemeral environment spinning up at http://34.220.15.121:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873524538
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
Not sure if I follow - it looks to me like rows and columns in pivot table do return to their original states?
https://user-images.githubusercontent.com/15073128/168563992-9d9749fb-61e4-4664-b0a5-f663a4ac40df.mov
Or do you mean that, for example, if both columns and rows in pivot table contain a value `name`, then it gets deduplicated when switching to table but `sourceControl` contains information that `name` originates from both `columns` and `rows` in pivot table? If that's the point, I like this idea!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872367864
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ sharedFormData[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardized_form_data?.memorizedFormData,
+ )
+ ? new Map(formData.standardized_form_data.memorizedFormData)
+ : new Map();
+ const vizType = formData.viz_type;
+ if (memorizedFormData.has(vizType)) {
+ memorizedFormData.delete(vizType);
+ }
+ memorizedFormData.set(vizType, formData);
+ this.sfd = {
+ sharedFormData,
+ memorizedFormData,
+ };
+ }
+
+ static getReversedMap() {
+ const reversedMap = new Map();
+ Object.entries(sharedControls).forEach(([key, names]) => {
+ names.forEach(name => {
+ reversedMap.set(name, key);
+ });
+ });
+ return reversedMap;
+ }
+
+ private getLatestFormData(vizType: string): QueryFormData {
+ if (this.sfd.memorizedFormData.has(vizType)) {
+ return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
+ }
+
+ return this.memorizedFormData.slice(-1)[0][1];
Review Comment:
The `memorizedFormData` will not be empty, it be initialized in the constructor.
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ sharedFormData[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardized_form_data?.memorizedFormData,
+ )
+ ? new Map(formData.standardized_form_data.memorizedFormData)
+ : new Map();
+ const vizType = formData.viz_type;
+ if (memorizedFormData.has(vizType)) {
+ memorizedFormData.delete(vizType);
+ }
+ memorizedFormData.set(vizType, formData);
+ this.sfd = {
+ sharedFormData,
+ memorizedFormData,
+ };
+ }
+
+ static getReversedMap() {
+ const reversedMap = new Map();
+ Object.entries(sharedControls).forEach(([key, names]) => {
+ names.forEach(name => {
+ reversedMap.set(name, key);
+ });
+ });
+ return reversedMap;
+ }
+
+ private getLatestFormData(vizType: string): QueryFormData {
+ if (this.sfd.memorizedFormData.has(vizType)) {
+ return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
+ }
+
+ return this.memorizedFormData.slice(-1)[0][1];
Review Comment:
The `memorizedFormData` will not be empty, it is initialized in the constructor.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872298084
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
Review Comment:
Do we need this copy? I think we don't modify `sourceFormData` anywhere
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1129479352
Can you elaborate on "save switching history and retrieve the latest form_data by the viz type"? Do you mean users may switch back and forth between viz types and you'd like to restore previous control values? I'm not sure if this use case is worth all the effort... You can always just reapply to/fromStandardizedFormData, even if this means some fields may not be able to be restored (percent metrics for example).
And wouldn't most users use the browser navigation anyways? As long as we create a new form_data_key and push a new url history, the history should be retained?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
ktmud commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1129483343
@zhaoyongjie Is 2 about allowing users to restore field values when they switch between chart types?
Would it suffice if we just keep the history in a reducer state as well? It could be a simple vizType to formData dict. A plugin's `fromStandardizedState` function will accept both the standardized state from current formData and the previous formData stored and decide how to merge them.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873730892
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
> I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
This is an interesting question. It is also a legacy of Superset's history. The `Percentage Metrics` is a post-processing **metric** rather than a **SQL base metric**. basically, it is not simply mapping from a source viz to a target viz. The same control is `Contribution Mode` in the Line chart V2.
I have said before, to solve this issue, I think we eventually need to move all post-processing(AA) into the metric and column so that we can do the real map between different charts.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873731893
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -340,11 +341,26 @@ export interface ControlPanelSectionConfig {
controlSetRows: ControlSetRow[];
}
+export interface SharedFormData {
Review Comment:
I like `standardizedState` or `normalizedState` 👍
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1128321226
/testenv up
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged PR #20010:
URL: https://github.com/apache/superset/pull/20010
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873690776
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.test.tsx:
##########
@@ -0,0 +1,267 @@
+/**
+ * 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.
+ */
+import { getChartControlPanelRegistry, SqlaFormData } from '@superset-ui/core';
+import TableChartPlugin from '@superset-ui/plugin-chart-table';
+import { BigNumberTotalChartPlugin } from '@superset-ui/plugin-chart-echarts';
+import { sections } from '@superset-ui/chart-controls';
+import {
+ StandardizedFormData,
+ sharedControls,
+ publicControls,
+} from './standardizedFormData';
+import { xAxisControl } from '../../../plugins/plugin-chart-echarts/src/controls';
+
+describe('should collect control values and create SFD', () => {
+ const sharedControlsFormData = {};
+ Object.entries(sharedControls).forEach(([, names]) => {
+ names.forEach(name => {
+ sharedControlsFormData[name] = name;
+ });
+ });
+ const publicControlsFormData = Object.fromEntries(
+ publicControls.map((name, idx) => [[name], idx]),
+ );
+ const sourceMockFormData: SqlaFormData = {
+ ...sharedControlsFormData,
+ ...publicControlsFormData,
+ datasource: '100__table',
+ viz_type: 'source_viz',
+ };
+ const sourceMockStore = {
+ form_data: sourceMockFormData,
+ controls: Object.fromEntries(
+ Object.entries(sourceMockFormData).map(([key, value]) => [
+ key,
+ { value },
+ ]),
+ ),
+ datasource: {
+ type: 'table',
+ columns: [],
+ },
+ };
+ beforeAll(() => {
+ getChartControlPanelRegistry().registerValue('source_viz', {
+ controlPanelSections: [
+ sections.advancedAnalyticsControls,
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls.map(control => [control]),
+ },
+ {
+ label: 'axis column',
+ controlSetRows: [xAxisControl],
+ },
+ ],
+ });
+ getChartControlPanelRegistry().registerValue('target_viz', {
+ controlPanelSections: [
+ sections.advancedAnalyticsControls,
+ {
+ label: 'transform controls',
+ controlSetRows: publicControls.map(control => [control]),
+ },
+ {
+ label: 'axis column',
+ controlSetRows: [[xAxisControl]],
+ },
+ ],
+ denormalizeFormData: (formData: SqlaFormData) => ({
+ ...formData,
+ columns: formData.standardized_form_data.sharedFormData.columns,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
+ }),
+ });
+ });
+
+ test('collect sharedControls', () => {
+ const sfd = new StandardizedFormData(sourceMockFormData);
+
+ expect(sfd.dumpSFD().sharedFormData.metrics).toEqual(
+ sharedControls.metrics.map(controlName => controlName),
+ );
+ expect(sfd.dumpSFD().sharedFormData.columns).toEqual(
+ sharedControls.columns.map(controlName => controlName),
+ );
+ });
+
+ test('should transform all publicControls', () => {
+ const sfd = new StandardizedFormData(sourceMockFormData);
+ const { formData } = sfd.transform('target_viz', sourceMockStore);
+ Object.entries(publicControlsFormData).forEach(([key]) => {
+ expect(formData).toHaveProperty(key);
+ });
+ Object.entries(sharedControls).forEach(([key, value]) => {
+ expect(formData[key]).toEqual(value);
+ });
+ });
+});
+
+describe('should transform form_data between table and bigNumberTotal', () => {
+ const tableVizFormData = {
+ datasource: '30__table',
+ viz_type: 'table',
+ time_grain_sqla: 'P1D',
+ time_range: 'No filter',
+ query_mode: 'aggregate',
+ groupby: ['name'],
+ metrics: ['count'],
+ all_columns: [],
+ percent_metrics: [],
+ adhoc_filters: [],
+ order_by_cols: [],
+ row_limit: 10000,
+ server_page_length: 10,
+ order_desc: true,
+ table_timestamp_format: 'smart_date',
+ show_cell_bars: true,
+ color_pn: true,
+ applied_time_extras: {},
+ url_params: {
+ form_data_key:
+ 'p3No_sqDW7k-kMTzlBPAPd9vwp1IXTf6stbyzjlrPPa0ninvdYUUiMC6F1iKit3Y',
+ dataset_id: '30',
+ },
+ };
+ const tableVizStore = {
+ form_data: tableVizFormData,
+ controls: {
+ datasource: {
+ value: '30__table',
+ },
+ viz_type: {
+ value: 'table',
+ },
+ slice_id: {},
+ cache_timeout: {},
+ url_params: {
+ value: {
+ form_data_key:
+ 'p3No_sqDW7k-kMTzlBPAPd9vwp1IXTf6stbyzjlrPPa0ninvdYUUiMC6F1iKit3Y',
+ dataset_id: '30',
+ },
+ },
+ granularity_sqla: {},
+ time_grain_sqla: {
+ value: 'P1D',
+ },
+ time_range: {
+ value: 'No filter',
+ },
+ query_mode: {
+ value: 'aggregate',
+ },
+ groupby: {
+ value: ['name'],
+ },
+ metrics: {
+ value: ['count'],
+ },
+ all_columns: {
+ value: [],
+ },
+ percent_metrics: {
+ value: [],
+ },
+ adhoc_filters: {
+ value: [],
+ },
+ timeseries_limit_metric: {},
+ order_by_cols: {
+ value: [],
+ },
+ server_pagination: {},
+ row_limit: {
+ value: 10000,
+ },
+ server_page_length: {
+ value: 10,
+ },
+ include_time: {},
+ order_desc: {
+ value: true,
+ },
+ show_totals: {},
+ emit_filter: {},
+ table_timestamp_format: {
+ value: 'smart_date',
+ },
+ page_length: {},
+ include_search: {},
+ show_cell_bars: {
+ value: true,
+ },
+ align_pn: {},
+ color_pn: {
+ value: true,
+ },
+ column_config: {},
+ conditional_formatting: {},
+ },
+ datasource: {
+ type: 'table',
+ columns: [],
+ },
+ };
+
+ beforeAll(() => {
+ getChartControlPanelRegistry().registerValue(
+ 'big_number_total',
+ new BigNumberTotalChartPlugin().controlPanel,
+ );
+ getChartControlPanelRegistry().registerValue(
+ 'table',
+ new TableChartPlugin().controlPanel,
+ );
+ });
+
+ test('transform', () => {
+ // table -> bigNumberTotal
+ const sfd = new StandardizedFormData(tableVizFormData);
+ const { formData: bntFormData, controlsState: btnControlsState } =
Review Comment:
Nice catch! it's a typo.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873730892
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -517,6 +517,11 @@ const config: ControlPanelConfig = {
],
},
],
+ denormalizeFormData: formData => ({
+ ...formData,
+ metrics: formData.standardized_form_data.sharedFormData.metrics,
Review Comment:
> I'm wondering if we should consider adding `percent_metrics` to the metrics array in `sharedFormData`? Let's say we have a table chart with just a single metric in `percentage_metrics` and change to Pie - currently the metric will be lost.
This is an interesting question. It is also a legacy of Superset's history. The `Percentage Metrics` is a post-processing **metric** rather than a **SQL base metric**. basically, it can not simply be mapping from a source viz to a target viz. The same control is `Contribution Mode` in the Line chart V2.
I have said before, to solve this issue, I think we eventually need to move all post-processing(AA) into the metric and column so that we can do the real map between different charts.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872367864
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ sharedFormData[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardized_form_data?.memorizedFormData,
+ )
+ ? new Map(formData.standardized_form_data.memorizedFormData)
+ : new Map();
+ const vizType = formData.viz_type;
+ if (memorizedFormData.has(vizType)) {
+ memorizedFormData.delete(vizType);
+ }
+ memorizedFormData.set(vizType, formData);
+ this.sfd = {
+ sharedFormData,
+ memorizedFormData,
+ };
+ }
+
+ static getReversedMap() {
+ const reversedMap = new Map();
+ Object.entries(sharedControls).forEach(([key, names]) => {
+ names.forEach(name => {
+ reversedMap.set(name, key);
+ });
+ });
+ return reversedMap;
+ }
+
+ private getLatestFormData(vizType: string): QueryFormData {
+ if (this.sfd.memorizedFormData.has(vizType)) {
+ return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
+ }
+
+ return this.memorizedFormData.slice(-1)[0][1];
Review Comment:
The `memorizedFormData` will not be empty, it must be initialized in the constructor.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on PR #20010:
URL: https://github.com/apache/superset/pull/20010#issuecomment-1126027869
> Thanks, Kamil. I also noticed this but I want to discuss it with you for this. Since the Pivot Table allows `Row` and `Columns` to have the same columns, I didn't deduplicate the name of columns when collecting sharedControl. The current solution also provides the same view between Table and PivotTable. So, I think that if we need to add the restriction on the SharedControl, we also need to add this in Pivot Table.
I think that we should deduplicate control values by default, as we never allow drag and dropping duplicated values. In the case of Pivot Table it's allowed because Rows and Columns are separate controls. So I think that when we transition from Pivot Table to any other chart we should join values from Rows and Columns, remove duplicates and populate Dimensions of the new viz with the result
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
kgabryje commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r872299058
##########
superset-frontend/src/explore/controlUtils/standardizedFormData.ts:
##########
@@ -0,0 +1,182 @@
+/**
+ * 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.
+ */
+import {
+ ensureIsArray,
+ getChartControlPanelRegistry,
+ QueryFormData,
+} from '@superset-ui/core';
+import {
+ ControlStateMapping,
+ SharedFormData,
+ iStandardizedFormData,
+} from '@superset-ui/chart-controls';
+import { getControlsState } from 'src/explore/store';
+import { getFormDataFromControls } from './getFormDataFromControls';
+
+export const sharedControls: Record<keyof SharedFormData, string[]> = {
+ metrics: ['metric', 'metrics', 'metric_2'],
+ columns: ['groupby', 'columns', 'groupbyColumns', 'groupbyRows'],
+};
+export const publicControls = [
+ // time section
+ 'granularity_sqla',
+ 'time_grain_sqla',
+ 'time_range',
+ // filters
+ 'adhoc_filters',
+ // subquery limit(series limit)
+ 'limit',
+ // order by clause
+ 'timeseries_limit_metric',
+ 'series_limit_metric',
+ // desc or asc in order by clause
+ 'order_desc',
+ // outer query limit
+ 'row_limit',
+ // x asxs column
+ 'x_axis',
+ // advanced analytics - rolling window
+ 'rolling_type',
+ 'rolling_periods',
+ 'min_periods',
+ // advanced analytics - time comparison
+ 'time_compare',
+ 'comparison_type',
+ // advanced analytics - resample
+ 'resample_rule',
+ 'resample_method',
+];
+
+export class StandardizedFormData {
+ private sfd: iStandardizedFormData;
+
+ constructor(sourceFormData: QueryFormData) {
+ /*
+ * Support form_data for smooth switching between different viz
+ * */
+ const sharedFormData = {
+ metrics: [],
+ columns: [],
+ };
+ const formData = { ...sourceFormData };
+ const reversedMap = StandardizedFormData.getReversedMap();
+
+ Object.entries(formData).forEach(([key, value]) => {
+ if (reversedMap.has(key)) {
+ sharedFormData[reversedMap.get(key)].push(...ensureIsArray(value));
+ }
+ });
+
+ const memorizedFormData = Array.isArray(
+ formData?.standardized_form_data?.memorizedFormData,
+ )
+ ? new Map(formData.standardized_form_data.memorizedFormData)
+ : new Map();
+ const vizType = formData.viz_type;
+ if (memorizedFormData.has(vizType)) {
+ memorizedFormData.delete(vizType);
+ }
+ memorizedFormData.set(vizType, formData);
+ this.sfd = {
+ sharedFormData,
+ memorizedFormData,
+ };
+ }
+
+ static getReversedMap() {
+ const reversedMap = new Map();
+ Object.entries(sharedControls).forEach(([key, names]) => {
+ names.forEach(name => {
+ reversedMap.set(name, key);
+ });
+ });
+ return reversedMap;
+ }
+
+ private getLatestFormData(vizType: string): QueryFormData {
+ if (this.sfd.memorizedFormData.has(vizType)) {
+ return this.sfd.memorizedFormData.get(vizType) as QueryFormData;
+ }
+
+ return this.memorizedFormData.slice(-1)[0][1];
Review Comment:
Should we add some null check in case `memorizedFormData` is empty?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #20010: feat: standardized form_data
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #20010:
URL: https://github.com/apache/superset/pull/20010#discussion_r873705941
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/controlPanel.ts:
##########
@@ -96,4 +96,8 @@ export default {
label: t('Number format'),
},
},
+ denormalizeFormData: formData => ({
+ ...formData,
+ metric: formData.standardized_form_data.sharedFormData.metrics[0],
Review Comment:
HAHA. I will rename it. Good catch!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org