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 2021/02/16 15:56:35 UTC
[GitHub] [superset] dpgaspar opened a new pull request #13153: fix: engines that don't support comments
dpgaspar opened a new pull request #13153:
URL: https://github.com/apache/superset/pull/13153
### SUMMARY
Some engines (like Elasticsearch) don't support comments on SQL statements.
Adds `allows_sql_comments` to the engine specs
### 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
----------------------------------------------------------------
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] [superset] codecov-io commented on pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13153:
URL: https://github.com/apache/superset/pull/13153#issuecomment-779969162
# [Codecov](https://codecov.io/gh/apache/superset/pull/13153?src=pr&el=h1) Report
> Merging [#13153](https://codecov.io/gh/apache/superset/pull/13153?src=pr&el=desc) (fc0f598) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `13.77%`.
> The diff coverage is `52.57%`.
[](https://codecov.io/gh/apache/superset/pull/13153?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #13153 +/- ##
===========================================
+ Coverage 53.06% 66.83% +13.77%
===========================================
Files 489 492 +3
Lines 17314 29034 +11720
Branches 4482 0 -4482
===========================================
+ Hits 9187 19404 +10217
- Misses 8127 9630 +1503
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| python | `66.83% <52.57%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/13153?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `73.19% <ø> (ø)` | |
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [...s/260bf0649a77\_migrate\_x\_dateunit\_in\_time\_range.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8yNjBiZjA2NDlhNzdfbWlncmF0ZV94X2RhdGV1bml0X2luX3RpbWVfcmFuZ2UucHk=) | `0.00% <0.00%> (ø)` | |
| [...ons/versions/41ce8799acc3\_rename\_pie\_label\_type.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy80MWNlODc5OWFjYzNfcmVuYW1lX3BpZV9sYWJlbF90eXBlLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `62.43% <4.34%> (ø)` | |
| [superset/views/datasource.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS5weQ==) | `88.70% <16.66%> (ø)` | |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `58.61% <27.27%> (ø)` | |
| [superset/charts/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2V4Y2VwdGlvbnMucHk=) | `92.85% <77.77%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `88.26% <87.50%> (ø)` | |
| ... and [936 more](https://codecov.io/gh/apache/superset/pull/13153/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13153?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/superset/pull/13153?src=pr&el=footer). Last update [5aa38ef...fc0f598](https://codecov.io/gh/apache/superset/pull/13153?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] [superset] villebro commented on a change in pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13153:
URL: https://github.com/apache/superset/pull/13153#discussion_r577560380
##########
File path: tests/sql_parse_tests.py
##########
@@ -732,3 +732,15 @@ def test_is_valid_cvas(self):
"""
parsed = ParsedQuery(query, strip_comments=True)
assert not parsed.is_valid_ctas()
+
+ def test_strip_comments_from_sql(self):
+ """Test that we are able to strip comments out of SQL stmts"""
+
+ assert (
+ strip_comments_from_sql("SELECT col1, col2 FROM table1")
+ == "SELECT col1, col2 FROM table1"
+ )
+ assert (
+ strip_comments_from_sql("SELECT col1, col2 FROM table1\n-- comment")
+ == "SELECT col1, col2 FROM table1\n"
+ )
Review comment:
Let's add one more:
```python
assert (
strip_comments_from_sql("SELECT '--abc' as abc, col2 FROM table1\n")
== "SELECT '--abc' as abc, col2 FROM table1\n"
)
````
----------------------------------------------------------------
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] [superset] dpgaspar commented on a change in pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #13153:
URL: https://github.com/apache/superset/pull/13153#discussion_r577563840
##########
File path: tests/sql_parse_tests.py
##########
@@ -732,3 +732,15 @@ def test_is_valid_cvas(self):
"""
parsed = ParsedQuery(query, strip_comments=True)
assert not parsed.is_valid_ctas()
+
+ def test_strip_comments_from_sql(self):
+ """Test that we are able to strip comments out of SQL stmts"""
+
+ assert (
+ strip_comments_from_sql("SELECT col1, col2 FROM table1")
+ == "SELECT col1, col2 FROM table1"
+ )
+ assert (
+ strip_comments_from_sql("SELECT col1, col2 FROM table1\n-- comment")
+ == "SELECT col1, col2 FROM table1\n"
+ )
Review comment:
done
----------------------------------------------------------------
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] [superset] dpgaspar merged pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #13153:
URL: https://github.com/apache/superset/pull/13153
----------------------------------------------------------------
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] [superset] villebro commented on a change in pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13153:
URL: https://github.com/apache/superset/pull/13153#discussion_r577352913
##########
File path: superset/db_engine_specs/base.py
##########
@@ -933,6 +933,9 @@ def execute(cls, cursor: Any, query: str, **kwargs: Any) -> None:
:param kwargs: kwargs to be passed to cursor.execute()
:return:
"""
+ if not cls.allows_sql_comments:
+ query = sql_parse.ParsedQuery(query).strip_comments()
Review comment:
I checked the description of `SQL_QUERY_MUTATOR`, and it only mentions that it's typically used to add comments - but not strictly restricted to that. If we want to block mutation from happening, we should rename the property to `allows_query_mutator` or similar to highlight that we're blocking that from happening rather than just stripping comments.
As this is only affecting a single engine, I feel this is the least invasive solution for now - we can always change this later if the overhead becomes an 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] [superset] villebro commented on a change in pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #13153:
URL: https://github.com/apache/superset/pull/13153#discussion_r577437763
##########
File path: superset/db_engine_specs/base.py
##########
@@ -933,6 +933,9 @@ def execute(cls, cursor: Any, query: str, **kwargs: Any) -> None:
:param kwargs: kwargs to be passed to cursor.execute()
:return:
"""
+ if not cls.allows_sql_comments:
+ query = sql_parse.ParsedQuery(query).strip_comments()
Review comment:
I support the idea of doing a quick char check, best of both worlds 👍 Also, I'm assuming this is a temporary fix, so we should be able to disable it when/if your request is implemented.
----------------------------------------------------------------
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] [superset] dpgaspar commented on a change in pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #13153:
URL: https://github.com/apache/superset/pull/13153#discussion_r577427687
##########
File path: superset/db_engine_specs/base.py
##########
@@ -933,6 +933,9 @@ def execute(cls, cursor: Any, query: str, **kwargs: Any) -> None:
:param kwargs: kwargs to be passed to cursor.execute()
:return:
"""
+ if not cls.allows_sql_comments:
+ query = sql_parse.ParsedQuery(query).strip_comments()
Review comment:
This needs to happen before any query execution on engines that don't support comments. Just instantiating `ParsedQuery` is much less intensive now, but yes some overhead exist.
We used to have default comments on SQLab for example, probably was considered safe since comments are an ANSI SQL 92 standard. `allows_query_mutator` is a good option but seems more evasive
I've made a feature request on opendistro SQL:
https://github.com/opendistro-for-elasticsearch/sql/issues/1051
^^^ Community support is awesome on opendistro SQL, and they are heavily developing and about to release a major change.
We can make a quick char check before instantiating `ParsedQuery`, not pretty, but would avoid unnecessary processing on all statements
----------------------------------------------------------------
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] [superset] willbarrett commented on a change in pull request #13153: fix: engines that don't support comments
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #13153:
URL: https://github.com/apache/superset/pull/13153#discussion_r577134151
##########
File path: superset/db_engine_specs/base.py
##########
@@ -933,6 +933,9 @@ def execute(cls, cursor: Any, query: str, **kwargs: Any) -> None:
:param kwargs: kwargs to be passed to cursor.execute()
:return:
"""
+ if not cls.allows_sql_comments:
+ query = sql_parse.ParsedQuery(query).strip_comments()
Review comment:
I seem to remember that parsing the SQL statements is computationally intensive - would it make sense to add this to the `process_statement` method instead to avoid an additional parsing pass on the query?
----------------------------------------------------------------
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