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 2020/06/19 22:48:32 UTC

[GitHub] [incubator-superset] ktmud opened a new pull request #10113: feat(viz): add query mode switch for table chart

ktmud opened a new pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113


   ### SUMMARY
   
   Rearrange form controls for Table chart, replace GROUP BY and NOT GROUP BY with a "Query Mode" switch.
   
   It was always not clear what does GROUP BY and NOT GROUP BY mean and that they are mutually exclusive. This tab switch component should make things a little clearer.
   
   Must wait for #10112 and apache-superset/superset-ui#609 .
   
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   #### Before
   
   <img src="https://user-images.githubusercontent.com/335541/85184188-a7716180-b243-11ea-83c9-0c449427b065.png" width="300">
   
   #### After 
   <img src="https://user-images.githubusercontent.com/335541/84823553-6410bc00-afd3-11ea-8148-3816702cd844.png" width="300">
   
   
   ### TEST PLAN
   
   Manual testing
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue:
   - [x] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] 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.

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] [incubator-superset] villebro commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-650071657


   @ktmud I tried restarting this a few times, but it appears some unit tests are failing. Cypress seems to be fine, despite being red now (flaky test).


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-649856158


   Had to run `npm dedupe` to remove duplicate `react-table` in `package-lock.json`, but it seems to have broken jsdom. Will push a fix soon.


----------------------------------------------------------------
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.

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] [incubator-superset] villebro commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r444714980



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       These should probably be placed in `utils/core.py` as
   ```python
   class QueryMode(str, Enum):
       RAW = "raw"
      AGGREGATE = "aggregate"
   ```
   I would also suggest adding a deprecated type for detecting query mode, which would look at the presence of `all_columns`, and flag as `raw` is present, and `aggregate` if not.

##########
File path: superset-frontend/src/explore/components/Control.jsx
##########
@@ -27,7 +27,10 @@ const controlTypes = Object.keys(controlMap);
 const propTypes = {
   actions: PropTypes.object.isRequired,
   name: PropTypes.string.isRequired,
-  type: PropTypes.oneOf(controlTypes).isRequired,
+  type: PropTypes.oneOfType([
+    PropTypes.oneOf(controlTypes).isRequired,
+    PropTypes.func.isRequired,
+  ]),

Review comment:
       Should this be
   ```suggestion
     type: PropTypes.oneOf([controlTypes, PropTypes.func]).isRequired,
   ```




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud merged pull request #10113: feat(viz): add query mode switch to table chart

Posted by GitBox <gi...@apache.org>.
ktmud merged pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113


   


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r444533962



##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.jsx
##########
@@ -47,42 +48,11 @@ class ControlPanelsContainer extends React.Component {
   constructor(props) {
     super(props);
 
-    this.getControlData = this.getControlData.bind(this);
     this.removeAlert = this.removeAlert.bind(this);
     this.renderControl = this.renderControl.bind(this);
     this.renderControlPanelSection = this.renderControlPanelSection.bind(this);
   }
 
-  getControlData(controlName) {
-    if (React.isValidElement(controlName)) {
-      return controlName;
-    }
-
-    const control = this.props.controls[controlName];
-    // Identifying mapStateToProps function to apply (logic can't be in store)
-    let mapF = controlConfigs[controlName].mapStateToProps;
-
-    // Looking to find mapStateToProps override for this viz type
-    const controlPanelConfig =
-      getChartControlPanelRegistry().get(this.props.controls.viz_type.value) ||
-      {};
-    const controlOverrides = controlPanelConfig.controlOverrides || {};
-    if (
-      controlOverrides[controlName] &&
-      controlOverrides[controlName].mapStateToProps
-    ) {
-      mapF = controlOverrides[controlName].mapStateToProps;
-    }
-    // Applying mapStateToProps if needed
-    if (mapF) {
-      return {
-        ...control,
-        ...mapF(this.props.exploreState, control, this.props.actions),
-      };
-    }
-    return control;
-  }

Review comment:
       All these logics are now handled by `expandControlConfig` from `superset-ui/chart-controls`.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445091726



##########
File path: superset-frontend/src/explore/components/Control.jsx
##########
@@ -27,7 +27,10 @@ const controlTypes = Object.keys(controlMap);
 const propTypes = {
   actions: PropTypes.object.isRequired,
   name: PropTypes.string.isRequired,
-  type: PropTypes.oneOf(controlTypes).isRequired,
+  type: PropTypes.oneOfType([
+    PropTypes.oneOf(controlTypes).isRequired,
+    PropTypes.func.isRequired,
+  ]),

Review comment:
       I think `oneOf` is different than `oneofType`. `controlTypes` are string values, but `PropTypes.func` or `PropTypes.func.isRequired` is type definition.




----------------------------------------------------------------
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.

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] [incubator-superset] rusackas commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445126193



##########
File path: superset/viz.py
##########
@@ -572,6 +574,51 @@ class TableViz(BaseViz):
     is_timeseries = False
     enforce_numerical_metrics = False
 
+    def process_metrics(self) -> None:
+        """Process form data and store parsed column configs.
+           1. Determine query mode based on form_data params.
+                - Use `query_mode` if it has a valid value
+                - Set as RAW mode if `all_columns` is set
+                - Otherwise defaults to AGG mode
+           2. Determine output columns based on query mode.
+        """
+        # Verify form data first: if not specifying query mode, then cannot have both
+        # GROUP BY and RAW COLUMNS.
+        fd = self.form_data
+        if (
+            not fd.get("query_mode")
+            and fd.get("all_columns")
+            and (fd.get("groupby") or fd.get("metrics") or fd.get("percent_metrics"))
+        ):
+            raise QueryObjectValidationError(
+                _(
+                    "Choose either fields to [Group By] and [Metrics] and/or "

Review comment:
       Found this error message a little confusing. Maybe something like:
   
   `You cannot use [Columns] controls simultaneously with a combination of [Group By]/[Metrics]/[Percentage Metrics] controls. Please choose one or the other.`




----------------------------------------------------------------
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.

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] [incubator-superset] codecov-commenter commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-650299034


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=h1) Report
   > Merging [#10113](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9de9e1c19d8606340efa9849b2176704e4916e9b&el=desc) will **decrease** coverage by `0.23%`.
   > The diff coverage is `98.21%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10113/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10113      +/-   ##
   ==========================================
   - Coverage   70.57%   70.33%   -0.24%     
   ==========================================
     Files         594      594              
     Lines       31461    31483      +22     
     Branches     3228     3222       -6     
   ==========================================
   - Hits        22205    22145      -60     
   - Misses       9140     9222      +82     
     Partials      116      116              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `52.38% <74.24%> (-1.19%)` | :arrow_down: |
   | #javascript | `59.42% <80.88%> (-0.19%)` | :arrow_down: |
   | #python | `70.40% <100.00%> (+<0.01%)` | :arrow_up: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...et-frontend/src/explore/controlPanels/sections.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9zZWN0aW9ucy5qc3g=) | `100.00% <ø> (ø)` | |
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `61.22% <ø> (ø)` | |
   | [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/welcome/DashboardTable.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3dlbGNvbWUvRGFzaGJvYXJkVGFibGUuanN4) | `75.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `78.37% <83.33%> (+0.11%)` | :arrow_up: |
   | [superset-frontend/src/explore/controlUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzLmpz) | `96.19% <96.42%> (-0.72%)` | :arrow_down: |
   | [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `93.10% <100.00%> (ø)` | |
   | [...perset-frontend/src/explore/components/Control.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sLmpzeA==) | `100.00% <100.00%> (ø)` | |
   | [.../src/explore/components/ControlPanelsContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLmpzeA==) | `92.75% <100.00%> (+14.49%)` | :arrow_up: |
   | [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `35.71% <100.00%> (+3.63%)` | :arrow_up: |
   | ... and [20 more](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=footer). Last update [9de9e1c...d5ab573](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-650300876


   > @ktmud I tried restarting this a few times, but it appears some unit tests are failing. Cypress seems to be fine, despite being red now (flaky test).
   
   Rebased and fixed another issue with js-dom upgrade and now it seems fine?


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445228994



##########
File path: superset/viz.py
##########
@@ -572,6 +574,51 @@ class TableViz(BaseViz):
     is_timeseries = False
     enforce_numerical_metrics = False
 
+    def process_metrics(self) -> None:
+        """Process form data and store parsed column configs.
+           1. Determine query mode based on form_data params.
+                - Use `query_mode` if it has a valid value
+                - Set as RAW mode if `all_columns` is set
+                - Otherwise defaults to AGG mode
+           2. Determine output columns based on query mode.
+        """
+        # Verify form data first: if not specifying query mode, then cannot have both
+        # GROUP BY and RAW COLUMNS.
+        fd = self.form_data
+        if (
+            not fd.get("query_mode")
+            and fd.get("all_columns")
+            and (fd.get("groupby") or fd.get("metrics") or fd.get("percent_metrics"))
+        ):
+            raise QueryObjectValidationError(
+                _(
+                    "Choose either fields to [Group By] and [Metrics] and/or "

Review comment:
       Updated. P.S. This is for legacy API, most users would probably not see this anyway.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-649028447


   Github heads are messed up because of rebase. Close and reopen to see if it fixes CI.


----------------------------------------------------------------
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.

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] [incubator-superset] villebro commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445025679



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       > Yeah, I was unsure about how to do string enum in Python, so went with the easiest route.
   > 
   > Should we have a `core/consts.py` file?
   
   Oh yes, it's been put off for too long. I think it's more pythonic to call it `constants.py`. @john-bodley any strong feelings of how we could consolidate constants, enums and perhaps type aliases in one easy to find place? Having constants under `utils/core.py` is slightly confusing.
   
   Anyway, I suggest not doing that refactor in this PR, let's do that separately. So I propose just throwing this under `utils/core.py` for now.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r443922651



##########
File path: superset/viz.py
##########
@@ -563,6 +567,46 @@ class TableViz(BaseViz):
     is_timeseries = False
     enforce_numerical_metrics = False
 
+    def process_metrics(self) -> None:
+        """Process form data and store parsed column configs.
+           1. Determine query mode based on form_data params.
+                - Use `query_mode` if it has a valid value
+                - Set as RAW mode if `all_columns` is set
+                - Otherwise defaults to AGG mode
+           2. Determine output columns based on query mode.
+        """
+        super().process_metrics()

Review comment:
       Refactor how data fields are processed for `TableViz`. In general, for all visualizations, we should process all inputs as soon as possible, store the parsed params somewhere, and refer it as the single source of truth in all future operations.




----------------------------------------------------------------
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.

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] [incubator-superset] villebro commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r444714980



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       These should probably be placed in `utils/core.py` as
   ```python
   class QueryMode(str, Enum):
       RAW = "raw"
       AGGREGATE = "aggregate"
   ```
   I would also suggest adding a deprecated type for detecting query mode (`AUTO`?), which would look at the presence of `all_columns`, and flag as `raw` is present, and `aggregate` if not.




----------------------------------------------------------------
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.

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] [incubator-superset] rusackas commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445126193



##########
File path: superset/viz.py
##########
@@ -572,6 +574,51 @@ class TableViz(BaseViz):
     is_timeseries = False
     enforce_numerical_metrics = False
 
+    def process_metrics(self) -> None:
+        """Process form data and store parsed column configs.
+           1. Determine query mode based on form_data params.
+                - Use `query_mode` if it has a valid value
+                - Set as RAW mode if `all_columns` is set
+                - Otherwise defaults to AGG mode
+           2. Determine output columns based on query mode.
+        """
+        # Verify form data first: if not specifying query mode, then cannot have both
+        # GROUP BY and RAW COLUMNS.
+        fd = self.form_data
+        if (
+            not fd.get("query_mode")
+            and fd.get("all_columns")
+            and (fd.get("groupby") or fd.get("metrics") or fd.get("percent_metrics"))
+        ):
+            raise QueryObjectValidationError(
+                _(
+                    "Choose either fields to [Group By] and [Metrics] and/or "

Review comment:
       Found this error message a little confusing. Maybe something like:
   
   `You cannot use [Columns] controls simultaneously with [Group By]/[Metrics]/[Percentage Metrics] controls. Please choose one or the other.`




----------------------------------------------------------------
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.

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] [incubator-superset] villebro commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445104955



##########
File path: superset-frontend/src/explore/components/Control.jsx
##########
@@ -27,7 +27,10 @@ const controlTypes = Object.keys(controlMap);
 const propTypes = {
   actions: PropTypes.object.isRequired,
   name: PropTypes.string.isRequired,
-  type: PropTypes.oneOf(controlTypes).isRequired,
+  type: PropTypes.oneOfType([
+    PropTypes.oneOf(controlTypes).isRequired,
+    PropTypes.func.isRequired,
+  ]),

Review comment:
       Whoops, my bad, didn't notice that.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445021376



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       Yeah, I was unsure about how to do string enum in Python, so went with the easiest route.
   
   Should we have a `core/consts.py` file? There is a `superset/contants.py` file, but nothing much is going on there. Maybe `QueryMode` would fit in this file?




----------------------------------------------------------------
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.

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] [incubator-superset] rusackas commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445116630



##########
File path: superset-frontend/src/explore/components/Control.jsx
##########
@@ -70,16 +75,18 @@ export default class Control extends React.PureComponent {
     this.setState({ hovered });
   }
   render() {
-    if (!this.props.type) return null; // this catches things like <hr/> elements (not a control!) shoved into the control panel configs.
-    const ControlType = controlMap[this.props.type];
+    const { type: controlType } = this.props;
+    if (!controlType) return null; // this catches things like <hr/> elements (not a control!) shoved into the control panel configs.

Review comment:
       Side note, those really rub me the wrong way. At some point, I hope to open a PR that provides an alternate solution to that.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r444609979



##########
File path: superset-frontend/spec/javascripts/explore/exploreActions_spec.js
##########
@@ -22,13 +22,13 @@ import exploreReducer from 'src/explore/reducers/exploreReducer';
 import * as actions from 'src/explore/actions/exploreActions';
 
 describe('reducers', () => {
-  it('sets correct control value given a key and value', () => {
+  it('sets correct control value given an arbitrary key and value', () => {
     const newState = exploreReducer(
       defaultState,
-      actions.setControlValue('x_axis_label', 'x', []),
+      actions.setControlValue('NEW_FIELD', 'x', []),
     );
-    expect(newState.controls.x_axis_label.value).toBe('x');
-    expect(newState.form_data.x_axis_label).toBe('x');
+    expect(newState.controls.NEW_FIELD.value).toBe('x');
+    expect(newState.form_data.NEW_FIELD).toBe('x');

Review comment:
       `x_axis_label` is not an active control at initialization or a shared control in global state. Use an ugly name to avoid confusion.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-648923571


   > We probably should add `query_mode` into `QueryObject` both here and on `@superset-ui/query`
   
   Do you mind if I do this in a separate PR?


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud closed pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud closed pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113


   


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r444603148



##########
File path: tests/viz_tests.py
##########
@@ -344,33 +344,28 @@ def test_adhoc_filters_overwrite_legacy_filters(self):
         self.assertEqual("(value3 in ('North America'))", query_obj["extras"]["where"])
         self.assertEqual("", query_obj["extras"]["having"])
 
-    @patch("superset.viz.BaseViz.query_obj")
-    def test_query_obj_merges_percent_metrics(self, super_query_obj):
+    def test_query_obj_merges_percent_metrics(self):
         datasource = self.get_datasource_mock()
         form_data = {
-            "percent_metrics": ["sum__A", "avg__B", "max__Y"],
             "metrics": ["sum__A", "count", "avg__C"],
+            "percent_metrics": ["sum__A", "avg__B", "max__Y"],
         }
         test_viz = viz.TableViz(datasource, form_data)
-        f_query_obj = {"metrics": form_data["metrics"]}
-        super_query_obj.return_value = f_query_obj
         query_obj = test_viz.query_obj()
         self.assertEqual(
             ["sum__A", "count", "avg__C", "avg__B", "max__Y"], query_obj["metrics"]
         )
 
-    @patch("superset.viz.BaseViz.query_obj")
-    def test_query_obj_throws_columns_and_metrics(self, super_query_obj):
+    def test_query_obj_throws_columns_and_metrics(self):
         datasource = self.get_datasource_mock()
         form_data = {"all_columns": ["A", "B"], "metrics": ["x", "y"]}
-        super_query_obj.return_value = {}
-        test_viz = viz.TableViz(datasource, form_data)
         with self.assertRaises(Exception):
+            test_viz = viz.TableViz(datasource, form_data)

Review comment:
       The exception now raises earlier.

##########
File path: superset-frontend/src/explore/controlUtils.js
##########
@@ -32,13 +34,13 @@ export function getFormDataFromControls(controlsState) {
   return formData;
 }
 
-export function validateControl(control) {
+export function validateControl(control, processedState) {
   const validators = control.validators;
   if (validators && validators.length > 0) {
     const validatedControl = { ...control };
     const validationErrors = [];
     validators.forEach(f => {
-      const v = f(control.value);
+      const v = f.call(control, control.value, processedState);

Review comment:
       Extend the `validators` API to allow access to processed state and the control config itself (not really in use anywhere yet). 

##########
File path: tests/viz_tests.py
##########
@@ -390,21 +385,23 @@ def test_query_obj_merges_all_columns(self, super_query_obj):
         self.assertEqual([], query_obj["groupby"])
         self.assertEqual([["colA", "colB"], ["colC"]], query_obj["orderby"])
 
-    @patch("superset.viz.BaseViz.query_obj")
-    def test_query_obj_uses_sortby(self, super_query_obj):
+    def test_query_obj_uses_sortby(self):
         datasource = self.get_datasource_mock()
-        form_data = {"timeseries_limit_metric": "__time__", "order_desc": False}
-        super_query_obj.return_value = {"metrics": ["colA", "colB"]}
+        form_data = {
+            "metrics": ["colA", "colB"],
+            "timeseries_limit_metric": "sort_column",
+            "order_desc": False,
+        }
         test_viz = viz.TableViz(datasource, form_data)
         query_obj = test_viz.query_obj()
-        self.assertEqual(["colA", "colB", "__time__"], query_obj["metrics"])
-        self.assertEqual([("__time__", True)], query_obj["orderby"])
+        self.assertEqual(["colA", "colB", "sort_column"], query_obj["metrics"])
+        self.assertEqual([("sort_column", True)], query_obj["orderby"])

Review comment:
       Rename so not to be confused with the time column (`__timestamp`).
   
   

##########
File path: tests/viz_tests.py
##########
@@ -344,26 +344,21 @@ def test_adhoc_filters_overwrite_legacy_filters(self):
         self.assertEqual("(value3 in ('North America'))", query_obj["extras"]["where"])
         self.assertEqual("", query_obj["extras"]["having"])
 
-    @patch("superset.viz.BaseViz.query_obj")

Review comment:
       Not sure why the patch is here, but removing it doesn't hurt.
   
   




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445085001



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       There is a `superset/contants.py` file, but nothing much seems going on there.
   
   Just added a `QueryMode` in `utils/core.py` as well as a `Enum.get` method to safeguard invalid enum values.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445021376



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       Yeah, I was unsure about how to do string enum in Python, so went with the easiest route.
   
   Should we have a `core/consts.py` file?




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445021376



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       Yeah, I was unsure about how to do string enum in Python, so went with the easiest route.
   
   Should we have a `core/consts.py` file?




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445231666



##########
File path: superset-frontend/src/explore/components/Control.jsx
##########
@@ -70,16 +75,18 @@ export default class Control extends React.PureComponent {
     this.setState({ hovered });
   }
   render() {
-    if (!this.props.type) return null; // this catches things like <hr/> elements (not a control!) shoved into the control panel configs.
-    const ControlType = controlMap[this.props.type];
+    const { type: controlType } = this.props;
+    if (!controlType) return null; // this catches things like <hr/> elements (not a control!) shoved into the control panel configs.

Review comment:
       I think this comment is inaccurate. The JSX elements and nulls should be handled by [ControlPanelContainer.jsx](https://github.com/ktmud/incubator-superset/blob/efd0314819b91ebc00af1cbd4e32cb0b997d40e1/superset-frontend/src/explore/components/ControlPanelsContainer.jsx#L132-L137) already.




----------------------------------------------------------------
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.

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] [incubator-superset] villebro commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-648925601


   > > We probably should add `query_mode` into `QueryObject` both here and on `@superset-ui/query`
   > 
   > Do you mind if I do this in a separate PR?
   
   @ktmud sure thing, just wanted to put the comments here to make sure I don't forget mentioning it. I can also do it, I have an idea of how we can do it with minimal disruption.


----------------------------------------------------------------
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.

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] [incubator-superset] rusackas commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445118204



##########
File path: superset-frontend/src/visualizations/presets/MainPreset.js
##########
@@ -39,7 +39,7 @@ import PivotTableChartPlugin from '@superset-ui/legacy-plugin-chart-pivot-table'
 import RoseChartPlugin from '@superset-ui/legacy-plugin-chart-rose';
 import SankeyChartPlugin from '@superset-ui/legacy-plugin-chart-sankey';
 import SunburstChartPlugin from '@superset-ui/legacy-plugin-chart-sunburst';
-import TableChartPlugin from '@superset-ui/legacy-plugin-chart-table';
+import TableChartPlugin from '@superset-ui/plugin-chart-table';

Review comment:
       🎉 




----------------------------------------------------------------
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.

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] [incubator-superset] codecov-commenter edited a comment on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-650299034


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=h1) Report
   > Merging [#10113](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/9de9e1c19d8606340efa9849b2176704e4916e9b&el=desc) will **decrease** coverage by `0.16%`.
   > The diff coverage is `98.21%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10113/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10113      +/-   ##
   ==========================================
   - Coverage   70.57%   70.41%   -0.17%     
   ==========================================
     Files         594      594              
     Lines       31461    31483      +22     
     Branches     3228     3222       -6     
   ==========================================
   - Hits        22205    22170      -35     
   - Misses       9140     9198      +58     
   + Partials      116      115       -1     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `52.80% <74.24%> (-0.77%)` | :arrow_down: |
   | #javascript | `59.42% <80.88%> (-0.19%)` | :arrow_down: |
   | #python | `70.40% <100.00%> (+<0.01%)` | :arrow_up: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...et-frontend/src/explore/controlPanels/sections.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9zZWN0aW9ucy5qc3g=) | `100.00% <ø> (ø)` | |
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `61.22% <ø> (ø)` | |
   | [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/welcome/DashboardTable.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3dlbGNvbWUvRGFzaGJvYXJkVGFibGUuanN4) | `75.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `78.37% <83.33%> (+0.11%)` | :arrow_up: |
   | [superset-frontend/src/explore/controlUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzLmpz) | `96.19% <96.42%> (-0.72%)` | :arrow_down: |
   | [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `93.10% <100.00%> (ø)` | |
   | [...perset-frontend/src/explore/components/Control.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sLmpzeA==) | `100.00% <100.00%> (ø)` | |
   | [.../src/explore/components/ControlPanelsContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLmpzeA==) | `92.75% <100.00%> (+14.49%)` | :arrow_up: |
   | [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `35.71% <100.00%> (+3.63%)` | :arrow_up: |
   | ... and [12 more](https://codecov.io/gh/apache/incubator-superset/pull/10113/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=footer). Last update [9de9e1c...d5ab573](https://codecov.io/gh/apache/incubator-superset/pull/10113?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r443922651



##########
File path: superset/viz.py
##########
@@ -563,6 +567,46 @@ class TableViz(BaseViz):
     is_timeseries = False
     enforce_numerical_metrics = False
 
+    def process_metrics(self) -> None:
+        """Process form data and store parsed column configs.
+           1. Determine query mode based on form_data params.
+                - Use `query_mode` if it has a valid value
+                - Set as RAW mode if `all_columns` is set
+                - Otherwise defaults to AGG mode
+           2. Determine output columns based on query mode.
+        """
+        super().process_metrics()

Review comment:
       Refactor how data fields are processed for `TableViz`. In general, for all visualizations, we should process all inputs as soon as possible, store the parsed params somewhere, and refer the single source of truth in all future operations.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on a change in pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#discussion_r445025296



##########
File path: superset/viz.py
##########
@@ -553,6 +554,10 @@ def json_data(self) -> str:
         return json.dumps(self.data)
 
 
+QUERY_MODE_RAW = "raw"
+QUERY_MODE_AGGREGATE = "aggregate"

Review comment:
       > I would also suggest adding a deprecated type for detecting query mode (`AUTO`?), which would look at the presence of `all_columns`, and flag as `raw` is present, and `aggregate` if not.
   
   The logic is there, it's just not its own query mode. Kind of feel it'd be more useful to add a `QueryMode.from_str(str)` method than introducing an `auto` mode.




----------------------------------------------------------------
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.

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] [incubator-superset] ktmud commented on pull request #10113: feat(viz): add query mode switch for table chart

Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10113:
URL: https://github.com/apache/incubator-superset/pull/10113#issuecomment-649855683


   @nytai I've also [upgraded](https://github.com/apache/incubator-superset/pull/10113/commits/149286cc3520be34a8d9e335f1d0b56cb7390562) `react-table` to `v7.2.1` because that's what the new table chart is using. The only change related to your ListView work is that the `sortable` prop for columns is now called `canSort`.


----------------------------------------------------------------
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.

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