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/05/20 07:07:32 UTC

[GitHub] [incubator-superset] villebro opened a new pull request #9854: fix: ignore aliases on where clauses

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


   ### SUMMARY
   On BigQuery, some WHERE clauses on nested fields of RECORD type columns were failing due to aliases being added to all columns in the WHERE clause. This is in violation of the BigQuery spec, but also more generally inconsistent with the ANSI SQL spec:
   > You cannot reference column aliases from the SELECT list in the WHERE clause.
   
   Source: https://cloud.google.com/bigquery/docs/reference/standard-sql/query-syntax#where_clause
   
   > Standard SQL disallows references to column aliases in a WHERE clause. This restriction is imposed because when the WHERE clause is evaluated, the column value may not yet have been determined.
   
   Source: https://dev.mysql.com/doc/refman/8.0/en/problems-with-alias.html
   
   This PR disables aliasing for WHERE clauses **for all engines**, and fixes the immediate problem for BigQuery. I propose getting this merged ASAP to fix the observed BigQuery bug, but propose adding unit tests later for this as we add proper tests for officially supported database engines.
   
   ### BEFORE
   ![image](https://user-images.githubusercontent.com/33317356/82415261-f24f5c00-9a80-11ea-8c10-436a97984b2b.png)
   Note the alias being referenced in the WHERE clause
   
   ### AFTER
   ![image](https://user-images.githubusercontent.com/33317356/82415194-d9df4180-9a80-11ea-8032-01f2bfbd39f4.png)
   Note the original field name being referenced in the WHERE clause, with the alias being referenced by the GROUPBY clause.
   
   ### TEST PLAN
   CI
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: closes #9836
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   FYI: @esilver


----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428236021



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I've played with this in the past by forcing `render_label_as_label=None` when calling the `super()`. For your scenario it seems like it may need to differ based on the context, i.e., within the `WHERE` clause. 
   
   For example we needed to ensure that the `ORDER BY` didn't contain the alias/label, so we did,
   
   ```python
   class MyCompilier(DruidCompiler):
       def visit_label(  # pylint: disable=too-many-arguments
           self,
           label: _CompileLabel,
           add_to_result_map: Optional[Callable] = None,
           within_label_clause: Optional[bool] = False,
           within_columns_clause: Optional[bool] = False,
           render_label_as_label: Optional[_CompileLabel] = None,
           **kwargs: Any,
       ) -> str:
           """
           Only render labels (aliases) within the column clause.
           Note this overrides the default behavior of rendering labels within the ORDER BY
           clause since the ORDER BY clause does not support aliases.
           """
   
           return super().visit_label(
               label,
               add_to_result_map=add_to_result_map,
               within_label_clause=within_label_clause,
               within_columns_clause=within_columns_clause,
               render_label_as_label=None,
               **kwargs,
           )
   ```
   
   which was tested (using `pytest`) via: 
   
   ```python
   from sqlalchemy.engine import Engine
   from sqlalchemy.sql import literal_column, select
   
   
   def test_identifier_preparer(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col])
       assert str(stmt) == "SELECT SUM(x) AS total"
       assert str(stmt.compile(engine)) == 'SELECT SUM(x) AS "total"'
   
   
   def test_visit_label(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col]).order_by(col)
       assert str(stmt).endswith("ORDER BY total")
       assert str(stmt.compile(engine)).endswith("ORDER BY SUM(x)")
   ```




----------------------------------------------------------------
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 #9854: fix: ignore aliases on where clauses

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



##########
File path: superset/connectors/sqla/models.py
##########
@@ -1068,7 +1080,7 @@ def mutator(df: pd.DataFrame) -> None:
             """
 
             labels_expected = query_str_ext.labels_expected
-            if df is not None and not df.empty:
+            if df is not None:

Review comment:
       During testing I noticed that column names were not being renamed back to their original format if the dataframe was 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.

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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428219361



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       @villebro do you think there is merit in trying to fix the BigQuery DB-API instead, i.e., overriding the [`visit_label`](https://github.com/sqlalchemy/sqlalchemy/blob/ea87d39d7a9926dc1c6bf3d70e8faf8575769cb0/lib/sqlalchemy/sql/compiler.py#L1147) method? Note I've had to do this for a custom dialect. 
   
   This would ensure that i) this would be fixed for all BigQuery use cases (and not just those emanating from Superset), and ii) reduce the need for custom Superset overrides. 




----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428236021



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I've played with this in the past by forcing `render_label_as_label=None` when calling the `super()`. I think [this](https://github.com/mxmzdlv/pybigquery/blob/master/pybigquery/sqlalchemy_bigquery.py#L189) may be the issue.




----------------------------------------------------------------
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 #9854: fix: ignore aliases on where clauses

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=h1) Report
   > Merging [#9854](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c117e222c09590bc56ad06ad1708e5d264b1c798&el=desc) will **decrease** coverage by `5.01%`.
   > The diff coverage is `88.23%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9854/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9854      +/-   ##
   ==========================================
   - Coverage   71.02%   66.01%   -5.02%     
   ==========================================
     Files         583      588       +5     
     Lines       30634    30611      -23     
     Branches     3165     3238      +73     
   ==========================================
   - Hits        21758    20207    -1551     
   - Misses       8765    10217    +1452     
   - Partials      111      187      +76     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.30% <ø> (-0.01%)` | :arrow_down: |
   | #python | `70.79% <88.23%> (-0.48%)` | :arrow_down: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.44% <88.23%> (-0.16%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupColors.js](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupFormatters.js](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [201 more](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9854?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/9854?src=pr&el=footer). Last update [c117e22...46ae952](https://codecov.io/gh/apache/incubator-superset/pull/9854?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] codecov-commenter edited a comment on pull request #9854: fix: ignore aliases on where clauses

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=h1) Report
   > Merging [#9854](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c117e222c09590bc56ad06ad1708e5d264b1c798&el=desc) will **increase** coverage by `0.12%`.
   > The diff coverage is `88.23%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9854/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9854      +/-   ##
   ==========================================
   + Coverage   71.02%   71.14%   +0.12%     
   ==========================================
     Files         583      588       +5     
     Lines       30634    30794     +160     
     Branches     3165     3238      +73     
   ==========================================
   + Hits        21758    21909     +151     
   - Misses       8765     8774       +9     
     Partials      111      111              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `54.05% <ø> (+0.55%)` | :arrow_up: |
   | #javascript | `59.30% <ø> (-0.01%)` | :arrow_down: |
   | #python | `71.27% <88.23%> (+<0.01%)` | :arrow_up: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.63% <88.23%> (+0.03%)` | :arrow_up: |
   | [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9BZGhvY01ldHJpY0VkaXRQb3BvdmVyVGl0bGUuanN4) | `84.61% <0.00%> (-8.72%)` | :arrow_down: |
   | [...ponents/AdhocFilterEditPopoverSimpleTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9BZGhvY0ZpbHRlckVkaXRQb3BvdmVyU2ltcGxlVGFiQ29udGVudC5qc3g=) | `86.50% <0.00%> (-7.20%)` | :arrow_down: |
   | [.../src/explore/components/AdhocFilterEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9BZGhvY0ZpbHRlckVkaXRQb3BvdmVyLmpzeA==) | `74.46% <0.00%> (-4.26%)` | :arrow_down: |
   | [...rontend/src/visualizations/FilterBox/FilterBox.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL0ZpbHRlckJveC9GaWx0ZXJCb3guanN4) | `70.83% <0.00%> (-3.34%)` | :arrow_down: |
   | [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `87.64% <0.00%> (-2.25%)` | :arrow_down: |
   | [.../src/explore/components/controls/SelectControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9TZWxlY3RDb250cm9sLmpzeA==) | `93.33% <0.00%> (-1.97%)` | :arrow_down: |
   | [...erset-frontend/src/components/ListView/Filters.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvRmlsdGVycy50c3g=) | `88.88% <0.00%> (-1.68%)` | :arrow_down: |
   | [...t-frontend/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZFN0YXRlLmpz) | `58.00% <0.00%> (-1.34%)` | :arrow_down: |
   | [superset-frontend/src/explore/controls.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbHMuanN4) | `80.95% <0.00%> (-1.20%)` | :arrow_down: |
   | ... and [43 more](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9854?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/9854?src=pr&el=footer). Last update [c117e22...46ae952](https://codecov.io/gh/apache/incubator-superset/pull/9854?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] codecov-commenter commented on pull request #9854: fix: ignore aliases on where clauses

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=h1) Report
   > Merging [#9854](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c117e222c09590bc56ad06ad1708e5d264b1c798&el=desc) will **decrease** coverage by `11.72%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9854/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master    #9854       +/-   ##
   ===========================================
   - Coverage   71.02%   59.30%   -11.73%     
   ===========================================
     Files         583      404      -179     
     Lines       30634    12741    -17893     
     Branches     3165     3238       +73     
   ===========================================
   - Hits        21758     7556    -14202     
   + Misses       8765     4998     -3767     
   - Partials      111      187       +76     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.30% <ø> (-0.01%)` | :arrow_down: |
   | #python | `?` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9854?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupColors.js](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupFormatters.js](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvaW5kZXguanM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [340 more](https://codecov.io/gh/apache/incubator-superset/pull/9854/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9854?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/9854?src=pr&el=footer). Last update [c117e22...46ae952](https://codecov.io/gh/apache/incubator-superset/pull/9854?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] villebro closed pull request #9854: [WIP] fix: ignore aliases on where clauses

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


   


----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428219361



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       @villebro do you think there is merit in trying to fix the BQ DB-API instead?




----------------------------------------------------------------
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 #9854: fix: ignore aliases on where clauses

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



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       Thanks for the pointers @john-bodley . I just forked it, will hack on it this weekend.




----------------------------------------------------------------
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 #9854: [WIP] fix: ignore aliases on where clauses

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


   @esilver I will be closing this for now, as this is a bug in `pybigquery`, hence it should be fixed there. I haven't gotten around to crafting a fix yet, but will try to find the time over the coming months.


----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428219361



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       @villebro do you think there is merit in trying to fix the BigQuery DB-API instead, i.e., overriding the [`visit_label`](https://github.com/obeattie/sqlalchemy/blob/376007fed7746d494dcb0166b22e512bfece02cd/lib/sqlalchemy/sql/compiler.py#L285) method? Note I've had to do this for a custom dialect. 
   
   This would ensure that i) this would be fixed for all BigQuery use cases (and not just those emanating from Superset), and ii) reduce the need for custom Superset overrides. 




----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428236021



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I've played with this in the past by forcing `render_label_as_label=None` when calling the `super()`. For your scenario it seems like it may need to differ based on the context, i.e., within the `WHERE` clause. 
   
   For example we needed to ensure that the `ORDER BY` didn't contain the alias/label, 
   
   ```python
   from sqlalchemy.engine import Engine
   from sqlalchemy.sql import literal_column, select
   
   
   def test_identifier_preparer(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col])
       assert str(stmt) == "SELECT SUM(x) AS total"
       assert str(stmt.compile(engine)) == 'SELECT SUM(x) AS "total"'
   
   
   def test_visit_label(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col]).order_by(col)
       assert str(stmt).endswith("ORDER BY total")
       assert str(stmt.compile(engine)).endswith("ORDER BY SUM(x)")
   ```




----------------------------------------------------------------
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 #9854: fix: ignore aliases on where clauses

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



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       Normally the dialect should do this, but apparently the BQ dialect isn't handling this properly (it does it correctly for all other operators except LIKE).




----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428236021



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I've played with this in the past by forcing `render_label_as_label=None` when calling the `super()`. For your scenario it seems like it may need to differ based on the context. 
   
   For example we needed to ensure that the `ORDER BY` didn't contain the alias, 
   
   ```
   from sqlalchemy.engine import Engine
   from sqlalchemy.sql import literal_column, select
   
   
   def test_identifier_preparer(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col])
       assert str(stmt) == "SELECT SUM(x) AS total"
       assert str(stmt.compile(engine)) == 'SELECT SUM(x) AS "total"'
   
   
   def test_visit_label(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col]).order_by(col)
       assert str(stmt).endswith("ORDER BY total")
       assert str(stmt.compile(engine)).endswith("ORDER BY SUM(x)")
   ```




----------------------------------------------------------------
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] esilver commented on pull request #9854: [WIP] fix: ignore aliases on where clauses

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


   makes sense, thanks!


----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428236021



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I've played with this in the past by forcing `render_label_as_label=None` when calling the `super()`. For your scenario it seems like it may need to differ based on the context, i.e., within the `WHERE` clause. 
   
   For example we needed to ensure that the `ORDER BY` didn't contain the alias, 
   
   ```python
   from sqlalchemy.engine import Engine
   from sqlalchemy.sql import literal_column, select
   
   
   def test_identifier_preparer(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col])
       assert str(stmt) == "SELECT SUM(x) AS total"
       assert str(stmt.compile(engine)) == 'SELECT SUM(x) AS "total"'
   
   
   def test_visit_label(engine: Engine) -> None:
       col = literal_column("SUM(x)").label("total")
       stmt = select([col]).order_by(col)
       assert str(stmt).endswith("ORDER BY total")
       assert str(stmt.compile(engine)).endswith("ORDER BY SUM(x)")
   ```




----------------------------------------------------------------
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 #9854: fix: ignore aliases on where clauses

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



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I thought about it, but laziness got the best of me. But why not, let me poke at it real quick and see if it can be fixed there.




----------------------------------------------------------------
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] john-bodley commented on a change in pull request #9854: fix: ignore aliases on where clauses

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9854:
URL: https://github.com/apache/incubator-superset/pull/9854#discussion_r428033535



##########
File path: superset/connectors/sqla/models.py
##########
@@ -167,15 +167,26 @@ def is_temporal(self) -> bool:
             self.type, utils.DbColumnType.TEMPORAL
         )
 
-    def get_sqla_col(self, label: Optional[str] = None) -> Column:
-        label = label or self.column_name
+    def get_sqla_col(

Review comment:
       I’m kind of surprised that SQLAlchemy isn’t dealing with the labels correctly. Are we not constructing the SQL query in the appropriate manner?

##########
File path: superset/connectors/sqla/models.py
##########
@@ -1068,7 +1080,7 @@ def mutator(df: pd.DataFrame) -> None:
             """
 
             labels_expected = query_str_ext.labels_expected
-            if df is not None and not df.empty:
+            if df is not None:

Review comment:
       Somewhat unrelated but I thought we banished `df` being `None`.




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