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/06 14:51:35 UTC

[GitHub] [incubator-superset] dpgaspar opened a new pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

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


   ### CATEGORY
   
   - [X] Bug Fix
   - [ ] Enhancement (new features, refinement)
   - [ ] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   On SQLLAb executing SQL statements with `CAST` and `AT` fail. This is a partial fix, since this seems to open up a bigger problem `sqlparse` does dot correctly identify columns with `CAST(FOO_COL AS TYPEX) AT TIME ZONE 'Eastern Standard Time'` for example, and just adds `CAST(FOO_OLD AS TYPEX)` on the IdentifierList.
   
   This is a partial fix, since it will discard `CAST` from the alias setting, so that we don't double alias it. Yet it means that all `CAST` statements need to have an Alias.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   


----------------------------------------------------------------
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 #9752: fix(mssql): reverts #9644 and displays a better error msg

Posted by GitBox <gi...@apache.org>.

villebro commented on a change in pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#discussion_r425084363



##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -85,6 +84,10 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
         return None
 
     @classmethod
-    def apply_limit_to_sql(cls, sql: str, limit: int, database: "Database") -> str:
-        new_sql = ParsedQuery(sql).set_alias()
-        return super().apply_limit_to_sql(new_sql, limit, database)
+    def extract_error_message(cls, ex: Exception) -> str:
+        if str(ex).startswith("(8155"):
+            return (
+                f"{cls.engine} error: All your SQL functions need to "
+                "have alias on MSSQL. For example: SELECT COUNT(*) AS C1 FROM TABLE1"

Review comment:
       ```suggestion
                   "have an alias on MSSQL. For example: SELECT COUNT(*) AS C1 FROM TABLE1"
   ```

##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -85,6 +84,10 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
         return None
 
     @classmethod
-    def apply_limit_to_sql(cls, sql: str, limit: int, database: "Database") -> str:
-        new_sql = ParsedQuery(sql).set_alias()
-        return super().apply_limit_to_sql(new_sql, limit, database)
+    def extract_error_message(cls, ex: Exception) -> str:
+        if str(ex).startswith("(8155"):

Review comment:
       ```suggestion
           if str(ex).startswith("(8155,"):
   ```
   You never know if they introduce `^\(8155\d+,`..

##########
File path: tests/db_engine_specs/mssql_tests.py
##########
@@ -97,64 +94,28 @@ def test_convert_dttm(self):
         for actual, expected in test_cases:
             self.assertEqual(actual, expected)
 
-    def test_apply_limit(self):
-        def compile_sqla_query(qry: Select, schema: Optional[str] = None) -> str:
-            return str(
-                qry.compile(
-                    dialect=mssql.dialect(), compile_kwargs={"literal_binds": True}
-                )
-            )
-
-        database = Database(
-            database_name="mssql_test",
-            sqlalchemy_uri="mssql+pymssql://sa:Password_123@localhost:1433/msdb",
+    def test_extract_error_message(self):
+        test_mssql_exception = Exception(
+            "(8155, b\"No column name was specified for column 1 of 'inner_qry'."
+            "DB-Lib error message 20018, severity 16:\\nGeneral SQL Server error: "
+            'Check messages from the SQL Server\\n")'
         )
-        db.session.add(database)
-        db.session.commit()
-
-        with mock.patch.object(database, "compile_sqla_query", new=compile_sqla_query):
-            test_sql = "SELECT COUNT(*) FROM FOO_TABLE"
-
-            limited_sql = MssqlEngineSpec.apply_limit_to_sql(test_sql, 1000, database)
-
-            expected_sql = (
-                "SELECT TOP 1000 * \n"
-                "FROM (SELECT COUNT(*) AS COUNT_1 FROM FOO_TABLE) AS inner_qry"
-            )
-            self.assertEqual(expected_sql, limited_sql)
-
-            test_sql = "SELECT COUNT(*), SUM(id) FROM FOO_TABLE"
-            limited_sql = MssqlEngineSpec.apply_limit_to_sql(test_sql, 1000, database)
-
-            expected_sql = (
-                "SELECT TOP 1000 * \n"
-                "FROM (SELECT COUNT(*) AS COUNT_1, SUM(id) AS SUM_2 FROM FOO_TABLE) "
-                "AS inner_qry"
-            )
-            self.assertEqual(expected_sql, limited_sql)
-
-            test_sql = "SELECT COUNT(*), FOO_COL1 FROM FOO_TABLE GROUP BY FOO_COL1"
-            limited_sql = MssqlEngineSpec.apply_limit_to_sql(test_sql, 1000, database)
-
-            expected_sql = (
-                "SELECT TOP 1000 * \n"
-                "FROM (SELECT COUNT(*) AS COUNT_1, "
-                "FOO_COL1 FROM FOO_TABLE GROUP BY FOO_COL1)"
-                " AS inner_qry"
-            )
-            self.assertEqual(expected_sql, limited_sql)
-
-            test_sql = "SELECT COUNT(*), COUNT(*) FROM FOO_TABLE"
-            limited_sql = MssqlEngineSpec.apply_limit_to_sql(test_sql, 1000, database)
-            expected_sql = (
-                "SELECT TOP 1000 * \n"
-                "FROM (SELECT COUNT(*) AS COUNT_1, COUNT(*) AS COUNT_2 FROM FOO_TABLE)"
-                " AS inner_qry"
-            )
-            self.assertEqual(expected_sql, limited_sql)
+        error_message = MssqlEngineSpec.extract_error_message(test_mssql_exception)
+        expected_message = (
+            "mssql error: All your SQL functions need to "
+            "have alias on MSSQL. For example: SELECT COUNT(*) AS C1 FROM TABLE1"

Review comment:
       ```suggestion
               "have an alias on MSSQL. For example: SELECT COUNT(*) AS C1 FROM TABLE1"
   ```




----------------------------------------------------------------
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] mistercrunch commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

mistercrunch commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625512069


   For reference, here's the FORCE_LIMIT logic:
   https://github.com/apache/incubator-superset/blob/13c5b133a9d054b07dff5bbcff987551e3c4c42e/superset/sql_parse.py#L255-L286
   
   FORCE_TOP wouldn't be that different. It's editing the SQL, but it's pretty safe (not as bad as squeezing aliases into subqueries...)


----------------------------------------------------------------
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] dpgaspar edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625398453


   @bkyryliuk we have a simple query construction here: https://github.com/apache/incubator-superset/blob/master/superset/db_engine_specs/base.py#L397 and it's being used for MSSQL. Is this something similar with what you're looking for?
   
   @mistercrunch also mentioned that using a `WRAP_SQL` is inefficient on some engines.
   
   My plan is, if feasible/simple change `WRAP_SQL` to something like `FORCE_TOP` and doing the best to identify non aliased functions and warn the user about it. This last one could be tricky for statements like: `SELECT COL1, COL1, CAST(COL3 AS DATETIME) AT TIME ZONE "SOME_TZ", COL4` because `sqlparse` seems to get lost on `AT` and does not identify `COL4` has a column.
   


----------------------------------------------------------------
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] dpgaspar edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625398453


   @bkyryliuk we have a simple query construction here: https://github.com/apache/incubator-superset/blob/master/superset/db_engine_specs/base.py#L397 and it's being used for MSSQL. Is this something similar with what you're looking for?
   
   @mistercrunch also mentioned that using a `WRAP_SQL` is inefficient on some engines.
   
   My plan is, if feasible/simple change `WRAP_SQL` to something like `FORCE_TOP` and doing the best to identify non aliased functions and warn the user about them. This last one could be tricky for statements like: `SELECT COL1, COL1, CAST(COL3 AS DATETIME) AT TIME ZONE "SOME_TZ", COL4` because `sqlparse` seems to get lost on `AT` and does not identify `COL4` has a column.
   


----------------------------------------------------------------
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-io commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

codecov-io commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625522233


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=h1) Report
   > Merging [#9752](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/67d8b634b885088b9977d04dc7dae1af8d180bfd&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9752/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9752      +/-   ##
   ==========================================
   + Coverage   70.76%   70.93%   +0.17%     
   ==========================================
     Files         585      183     -402     
     Lines       30429    17847   -12582     
     Branches     3107        0    -3107     
   ==========================================
   - Hits        21533    12660    -8873     
   + Misses       8782     5187    -3595     
   + Partials      114        0     -114     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `?` | |
   | #python | `70.93% <100.00%> (+<0.01%)` | :arrow_up: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/datasets/api.py](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvYXBpLnB5) | `92.56% <ø> (ø)` | |
   | [superset/charts/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL3NjaGVtYXMucHk=) | `100.00% <100.00%> (ø)` | |
   | [superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `71.49% <100.00%> (+0.07%)` | :arrow_up: |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | | |
   | [...t-frontend/src/explore/controlPanels/Partition.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9QYXJ0aXRpb24uanN4) | | |
   | [...nd/src/dashboard/util/getFilterScopeParentNodes.js](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlUGFyZW50Tm9kZXMuanM=) | | |
   | [...rontend/src/SqlLab/components/AceEditorWrapper.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIudHN4) | | |
   | [...rontend/src/dashboard/components/dnd/handleDrop.js](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2RuZC9oYW5kbGVEcm9wLmpz) | | |
   | [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | | |
   | [...perset-frontend/src/middleware/loggerMiddleware.js](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL21pZGRsZXdhcmUvbG9nZ2VyTWlkZGxld2FyZS5qcw==) | | |
   | ... and [370 more](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9752?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/9752?src=pr&el=footer). Last update [67d8b63...ed4adc8](https://codecov.io/gh/apache/incubator-superset/pull/9752?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] bkyryliuk commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

bkyryliuk commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625430874


   I like the approach, and eventually it would be nice to have that logic in the db specific engines.
   I briefly looked into explain and it doesn't look promising, https://docs.microsoft.com/en-us/sql/t-sql/queries/explain-transact-sql?view=azure-sqldw-latest
   
   Another potential alternative could be to catch an error and make it more user friendly e.g. stating that all columns should be aliased. It would make user experience slightly worse in the beginning, but over time should not be an issue and may be slightly easier to implement.


----------------------------------------------------------------
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] dpgaspar merged pull request #9752: fix(mssql): reverts #9644 and displays a better error msg

Posted by GitBox <gi...@apache.org>.

dpgaspar merged pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752


   


----------------------------------------------------------------
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 #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

villebro commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-624716939


   This is really a can of worms, and the deeper we go the more `sqlparse`-like logic we'll be introducing into Superset with unpleasant maintenance burden. I would almost propose deprecating advanced limiting for `TOP`-syntax dialects.


----------------------------------------------------------------
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] dpgaspar commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625398453


   @bkyryliuk we have a simple query construction here: https://github.com/apache/incubator-superset/blob/master/superset/db_engine_specs/base.py#L397 and it's being used for MSSQL. Is something similar what you're looking for?
   
   @mistercrunch also mentioned that using a `WRAP_SQL` is inefficient on some engines.
   
   My plan is, if feasible/simple change `WRAP_SQL` to something like `FORCE_TOP` and doing the best to identify non aliased functions and warn the user about it. This last one could be tricky for statements like: `SELECT COL1, COL1, CAST(COL3 AS DATETIME) AT TIME ZONE "SOME_TZ", COL4` `sqlparse` get seems to get lost on `AT` and does not identify `COL4` has a column.
   


----------------------------------------------------------------
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] dpgaspar edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625398453


   @bkyryliuk we have a simple query construction here: https://github.com/apache/incubator-superset/blob/master/superset/db_engine_specs/base.py#L397 and it's being used for MSSQL. Is this something similar with what you're looking for?
   
   @mistercrunch also mentioned that using a `WRAP_SQL` is inefficient on some engines.
   
   My plan is, if feasible/simple change `WRAP_SQL` to something like `FORCE_TOP` and doing the best to identify non aliased functions and warn the user about it. This last one could be tricky for statements like: `SELECT COL1, COL1, CAST(COL3 AS DATETIME) AT TIME ZONE "SOME_TZ", COL4` `sqlparse` get seems to get lost on `AT` and does not identify `COL4` has a column.
   


----------------------------------------------------------------
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] dpgaspar edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625398453


   @bkyryliuk we have a simple query construction here: https://github.com/apache/incubator-superset/blob/master/superset/db_engine_specs/base.py#L397 and it's being used for MSSQL. Is something similar with what you're looking for?
   
   @mistercrunch also mentioned that using a `WRAP_SQL` is inefficient on some engines.
   
   My plan is, if feasible/simple change `WRAP_SQL` to something like `FORCE_TOP` and doing the best to identify non aliased functions and warn the user about it. This last one could be tricky for statements like: `SELECT COL1, COL1, CAST(COL3 AS DATETIME) AT TIME ZONE "SOME_TZ", COL4` `sqlparse` get seems to get lost on `AT` and does not identify `COL4` has a column.
   


----------------------------------------------------------------
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] dpgaspar edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-624790056


   thks for the ideas @mistercrunch, we had/have a couple of problems with MSSQL on SQLLab. 
    1 - TOP wrapping was not working properly
    2 - Even without TOP wrapping, functions on MSSQL like `COUNT`, `SUM` etc are set has `<anonymous>` and these are not supported by superset (pandas dataframes?)
   
   Given that, maybe 2 is a good approach, or a mix of 1 and 2. May be wrong here, but still think that altering SQL statements is dangerous/difficult and kind of out of scope for superset. Another option could be not forcing `TOP` on some engines but refusing to execute statements that don't have them? 
   
   Note: This is scoped to MSSQL, because it was an obvious risky change
   


----------------------------------------------------------------
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] dpgaspar commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625534249


   Yes I know and totally agree @mistercrunch bad initial approach on my side.
   
   @bkyryliuk catching the error may be a good approach, could turn out to be difficult to pin the engine error, specific to non aliased functions, on the other hand, it's also a bit hard to parse the query and search for non aliased functions. So I may give it a try


----------------------------------------------------------------
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] mistercrunch commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

mistercrunch commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-624735784

The problem is not as much `TOP` as it is the requirement for alias in subquery.

And oh wow! I didn't know we were altering the SQL (as opposed to just wrapping it). That's pretty scary. Whenever parsing or alerting SQL is part of the solution, it's pretty scary. I'd advocate for even rolling back the logic that's in there currently (from before this PR).

Also if this logic should be anywhere, it should be scoped to MSSQL in `db_engine_spec` module

Few other options:
1. force a TOP clause (similar to `LimitMethod.FORCE_LIMIT`) it is altering the SQL, but somewhat less risky than squeezing aliases in. Some optimizers will do better with that than with the `LimitMethod.WRAP_SQL` approach.
1. surface a good error message to user "please alias all of your columns" and move forward with `LimitMethod.WRAP_SQL`
1. use `limit_method = LimitMethod.FETCH_MANY`, this has bad perf implications (usually means a larger "server side cursor" than necessary)
1. long shot: look for a way to let the server know ahead of time, some sort of cursor hint or parameter. That's clearly out of the DBAPI specification but it may exist

Personally I think 1 is best. It has the best perf guarantees (gives the clearest declarative insight to the db optimizer) and requires limited query alteration. When we moved forward with this a while ago I remember we looked at how other tools did this but forgot the details, maybe @bkyryliuk remembers?

----------------------------------------------------------------
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] dpgaspar commented on a change in pull request #9752: fix(mssql): reverts #9644 and displays a better error msg

Posted by GitBox <gi...@apache.org>.

dpgaspar commented on a change in pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#discussion_r425176265



##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -85,6 +84,10 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
         return None
 
     @classmethod
-    def apply_limit_to_sql(cls, sql: str, limit: int, database: "Database") -> str:
-        new_sql = ParsedQuery(sql).set_alias()
-        return super().apply_limit_to_sql(new_sql, limit, database)
+    def extract_error_message(cls, ex: Exception) -> str:
+        if str(ex).startswith("(8155"):

Review comment:
       good point




----------------------------------------------------------------
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] bkyryliuk commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

bkyryliuk commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625370820


   yeah we've looked into alternatives quite some time ago and sqlparse was the only feasible option at a time. There are DB specific parsers, but they often implemented in different languages e.g. java + antlr for hive and presto as I remember and using them as was mentioned will be opening a can of warms. Explain query can provide some useful information as well, however I am not sure about MS SQL specifics there. I'm curious what is a sqllachemy solution if it supports query construction for MSSQL


----------------------------------------------------------------
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-io edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-625522233


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=h1) Report
   > Merging [#9752](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/67d8b634b885088b9977d04dc7dae1af8d180bfd&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9752/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9752      +/-   ##
   ==========================================
   + Coverage   70.76%   70.93%   +0.17%     
   ==========================================
     Files         585      183     -402     
     Lines       30429    17847   -12582     
     Branches     3107        0    -3107     
   ==========================================
   - Hits        21533    12660    -8873     
   + Misses       8782     5187    -3595     
   + Partials      114        0     -114     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `?` | |
   | #python | `70.93% <100.00%> (+<0.01%)` | :arrow_up: |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9752?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/datasets/api.py](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YXNldHMvYXBpLnB5) | `92.56% <ø> (ø)` | |
   | [superset/charts/schemas.py](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL3NjaGVtYXMucHk=) | `100.00% <100.00%> (ø)` | |
   | [superset/viz.py](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `71.49% <100.00%> (+0.07%)` | :arrow_up: |
   | [...src/SqlLab/components/ExploreCtasResultsButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVDdGFzUmVzdWx0c0J1dHRvbi5qc3g=) | | |
   | [...frontend/src/dashboard/components/MissingChart.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL01pc3NpbmdDaGFydC5qc3g=) | | |
   | [...ore/components/controls/AnnotationLayerControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Bbm5vdGF0aW9uTGF5ZXJDb250cm9sLmpzeA==) | | |
   | [...et-frontend/src/components/Menu/LanguagePicker.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVudS9MYW5ndWFnZVBpY2tlci5qc3g=) | | |
   | [...d/src/explore/components/MetricDefinitionValue.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9NZXRyaWNEZWZpbml0aW9uVmFsdWUuanN4) | | |
   | [...ontend/src/dashboard/components/menu/HoverMenu.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL21lbnUvSG92ZXJNZW51LmpzeA==) | | |
   | [...explore/components/controls/ColorPickerControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db2xvclBpY2tlckNvbnRyb2wuanN4) | | |
   | ... and [370 more](https://codecov.io/gh/apache/incubator-superset/pull/9752/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9752?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/9752?src=pr&el=footer). Last update [67d8b63...ed4adc8](https://codecov.io/gh/apache/incubator-superset/pull/9752?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] dpgaspar edited a comment on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar edited a comment on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-624790056


   thks for the ideas @mistercrunch, we had/have a couple of problems with MSSQL on SQLLab. 
    1 - TOP wrapping was not working properly
    2 - Even without TOP wrapping, functions on MSSQL like `COUNT`, `SUM` etc are set has `<anonymous>` and these are not supported by superset (pandas dataframes?)
   
   Given that, maybe 2 is a good approach, or a mix of 1 and 2. May be wrong here, but still think that altering SQL statements is dangerous/difficult and kind of out of scope for superset. Another option could be: not forcing `TOP` on some engines but refusing to execute statements that don't have them? 
   
   Note: This is scoped to MSSQL, because it was an obvious risky change
   


----------------------------------------------------------------
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] dpgaspar commented on pull request #9752: fix(mssql): CAST with AT produces statements with syntax errors

Posted by GitBox <gi...@apache.org>.

dpgaspar commented on pull request #9752:
URL: https://github.com/apache/incubator-superset/pull/9752#issuecomment-624790056


   thks for the ideas @mistercrunch, we had/have a couple of problems with MSSQL on SQLLab. 
    1 - TOP wrapping was not working properly
    2 - Even without TOP wrapping, functions on MSSQL like `COUNT`, `SUM` etc are set has `<anonymous> and these are not supported by superset (pandas dataframes?)
   
   Given that, maybe 2 is a good approach, or a mix of 1 and 2. May be wrong here, but still think that altering SQL statements is dangerous/difficult and kind of out of scope for superset. Another option could be not forcing `TOP` on some engines but refusing to execute statements that don't have them? 
   
   Note: This is scoped to MSSQL, because it was an obvious risky change
   


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