You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/06/16 17:43:59 UTC
[GitHub] [incubator-superset] nytai opened a new pull request #10070: Tai/fix int parsing sqllab
nytai opened a new pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-commenter commented on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644928557
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=h1) Report
> Merging [#10070](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/d3a9ce5afc46ff45adb63f6f06856fa6a29f865c&el=desc) will **decrease** coverage by `0.18%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10070 +/- ##
==========================================
- Coverage 70.49% 70.30% -0.19%
==========================================
Files 585 585
Lines 31073 31074 +1
Branches 3185 3185
==========================================
- Hits 21905 21847 -58
- Misses 9058 9112 +54
- Partials 110 115 +5
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `53.14% <ø> (-0.76%)` | :arrow_down: |
| #javascript | `59.50% <ø> (ø)` | |
| #python | `70.07% <100.00%> (+<0.01%)` | :arrow_up: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `76.35% <100.00%> (+0.01%)` | :arrow_up: |
| [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <0.00%> (-16.67%)` | :arrow_down: |
| [superset-frontend/src/reduxUtils.ts](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlZHV4VXRpbHMudHM=) | `70.88% <0.00%> (-8.87%)` | :arrow_down: |
| [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `75.52% <0.00%> (-6.30%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `61.13% <0.00%> (-5.03%)` | :arrow_down: |
| [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `44.00% <0.00%> (-4.00%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `37.44% <0.00%> (-3.30%)` | :arrow_down: |
| [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `88.57% <0.00%> (-2.86%)` | :arrow_down: |
| [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `53.84% <0.00%> (-1.29%)` | :arrow_down: |
| [...rontend/src/SqlLab/components/AceEditorWrapper.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIudHN4) | `55.91% <0.00%> (-1.08%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10070?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/10070?src=pr&el=footer). Last update [d3a9ce5...0decfe9](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645105007
Shouldn't the endpoint accept `float` anyway though?
```python
@expose("/queries/<float:last_updated_ms>")
def queries(self, last_updated_ms: float) -> FlaskResponse:
```
Fraction millisecond sounds legit.
Then it also kind of makes sense for `/queries/abc` to return 404, since it looks like a totally different route.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud removed a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud removed a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645150700
@john-bodley Just tested locally and this works:
```diff
diff --git a/superset/views/core.py b/superset/views/core.py
index e1739721..b323e3a0 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -2658,17 +2658,14 @@ class Superset(BaseSupersetView):
return json_success(json.dumps(datasource.data))
@has_access_api
- @expose("/queries/<last_updated_ms>")
- def queries(self, last_updated_ms: str) -> FlaskResponse:
+ @expose("/queries/<int:last_updated_ms>")
+ @expose("/queries/<float:last_updated_ms>")
+ def queries(self, last_updated_ms: Union[int, float]) -> FlaskResponse:
"""
Get the updated queries.
:param last_updated_ms: unix time, milliseconds
"""
- last_updated_ms_int = int(float(last_updated_ms)) if last_updated_ms else 0
- return self.queries_exec(last_updated_ms_int)
-
- def queries_exec(self, last_updated_ms: int) -> FlaskResponse:
stats_logger.incr("queries")
if not g.user.get_id():
return json_error_response(
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645150700
@john-bodley Just tested locally and this works:
```diff
diff --git a/superset/views/core.py b/superset/views/core.py
index e1739721..b323e3a0 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -2658,17 +2658,14 @@ class Superset(BaseSupersetView):
return json_success(json.dumps(datasource.data))
@has_access_api
- @expose("/queries/<last_updated_ms>")
- def queries(self, last_updated_ms: str) -> FlaskResponse:
+ @expose("/queries/<int:last_updated_ms>")
+ @expose("/queries/<float:last_updated_ms>")
+ def queries(self, last_updated_ms: Union[int, float]) -> FlaskResponse:
"""
Get the updated queries.
:param last_updated_ms: unix time, milliseconds
"""
- last_updated_ms_int = int(float(last_updated_ms)) if last_updated_ms else 0
- return self.queries_exec(last_updated_ms_int)
-
- def queries_exec(self, last_updated_ms: int) -> FlaskResponse:
stats_logger.incr("queries")
if not g.user.get_id():
return json_error_response(
```
----------------------------------------------------------------
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] nytai edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644922204
@john-bodley We could, but since javascript doesn't really have integer support it's likely that these requests will happen again sooner or later. In either case I'm not sure 404 is what we want to do in this case, it's rather confusing from a client perspective, a 400(bad request) would make more sense.
----------------------------------------------------------------
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 #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#discussion_r441036437
##########
File path: superset/views/core.py
##########
@@ -2658,14 +2658,15 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
return json_success(json.dumps(datasource.data))
@has_access_api
- @expose("/queries/<int:last_updated_ms>")
- def queries(self, last_updated_ms: int) -> FlaskResponse:
+ @expose("/queries/<last_updated_ms>")
+ def queries(self, last_updated_ms: str) -> FlaskResponse:
"""
Get the updated queries.
:param last_updated_ms: unix time, milliseconds
"""
- return self.queries_exec(last_updated_ms)
+ last_updated_ms_int = int(float(last_updated_ms)) if last_updated_ms else 0
Review comment:
I wonder if there's a cleaner way of doing `int(float(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] ktmud edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645105007
Shouldn't the endpoint accept `float` anyway though?
```python
@expose("/queries/<float:last_updated_ms>")
def queries(self, last_updated_ms: float) -> FlaskResponse:
```
Fraction milliseconds seems legit.
And it also kind of makes sense for `/queries/abc` to return 404...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645150700
@john-bodley Just tested locally and this works:
```diff
diff --git a/superset/views/core.py b/superset/views/core.py
index e1739721..c87c6e5b 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -2658,17 +2658,17 @@ class Superset(BaseSupersetView):
return json_success(json.dumps(datasource.data))
@has_access_api
- @expose("/queries/<last_updated_ms>")
- def queries(self, last_updated_ms: str) -> FlaskResponse:
+ @expose("/queries/<int:last_updated_ms>")
+ @expose("/queries/<float:last_updated_ms>")
+ def queries(self, last_updated_ms: Union[int, float]) -> FlaskResponse:
"""
Get the updated queries.
:param last_updated_ms: unix time, milliseconds
"""
- last_updated_ms_int = int(float(last_updated_ms)) if last_updated_ms else 0
- return self.queries_exec(last_updated_ms_int)
+ return self.queries_exec(last_updated_ms)
- def queries_exec(self, last_updated_ms: int) -> FlaskResponse:
+ def queries_exec(self, last_updated_ms: Union[int, float]) -> FlaskResponse:
stats_logger.incr("queries")
if not g.user.get_id():
return json_error_response(
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645105007
Shouldn't the endpoint accept `float` anyway though?
```python
@expose("/queries/<float:last_updated_ms>")
def queries(self, last_updated_ms: float) -> FlaskResponse:
```
Fraction millisecond sounds legit.
And it also kind of makes sense for `/queries/abc` to return 404...
----------------------------------------------------------------
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 pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645141485
Actually @ktmud it seems like `float` isn't valid as per [here](https://github.com/pallets/werkzeug/blob/237b35a63631fda0fe80eb51dccdfe8a2f50d304/src/werkzeug/routing.py#L1277) (and the associated regex) it _only_ supports floating point numbers whereas ideally we would want to handle both floats and integers as far as I can tell.
----------------------------------------------------------------
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] nytai commented on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644981761
Merging this as is. Follow up improvements are welcome, but this issue is currently causing failures in production for us, so keeping the scope to reverting changes introduced in #9939
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645105007
Shouldn't the endpoint accept `float` anyway?
```python
@expose("/queries/<float:last_updated_ms>")
def queries(self, last_updated_ms: float) -> FlaskResponse:
```
Fraction milliseconds seems like legit computation.
And it also kind of makes sense for `/queries/abc` to return 404...
----------------------------------------------------------------
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 removed a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
john-bodley removed a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645141485
Actually @ktmud it seems like `float` isn't valid as per [here](https://github.com/pallets/werkzeug/blob/237b35a63631fda0fe80eb51dccdfe8a2f50d304/src/werkzeug/routing.py#L1277) (and the associated regex) it _only_ supports floating point numbers whereas ideally we would want to handle both floats and integers as far as I can tell.
----------------------------------------------------------------
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] nytai commented on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644922204
@john-bodley We could, but since javascript doesn't really have integer support it's likely that these requests will keep happening. In either case I'm not sure 404 is what we want to do in this case, it's rather confusing from a client perspective.
----------------------------------------------------------------
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 pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-645107420
@ktmud I agree and saw that Flask supports the `float` [url converter](https://exploreflask.com/en/latest/views.html#built-in-converters). I'll draft a PR for this.
----------------------------------------------------------------
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] nytai merged pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
nytai merged pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070
----------------------------------------------------------------
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 #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644928557
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=h1) Report
> Merging [#10070](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/d3a9ce5afc46ff45adb63f6f06856fa6a29f865c&el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10070 +/- ##
=======================================
Coverage 70.49% 70.49%
=======================================
Files 585 585
Lines 31073 31074 +1
Branches 3185 3185
=======================================
+ Hits 21905 21906 +1
Misses 9058 9058
Partials 110 110
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `53.89% <ø> (ø)` | |
| #javascript | `59.50% <ø> (ø)` | |
| #python | `70.07% <100.00%> (+<0.01%)` | :arrow_up: |
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10070?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10070/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `76.35% <100.00%> (+0.01%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10070?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/10070?src=pr&el=footer). Last update [d3a9ce5...0decfe9](https://codecov.io/gh/apache/incubator-superset/pull/10070?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] nytai edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644922204
@john-bodley We could, but since javascript doesn't really have integer support it's likely that these requests will happen again sooner or later. In either case I'm not sure 404 is what we want to do in this case, it's rather confusing from a client perspective. A 400 (bad request) would make more sense.
----------------------------------------------------------------
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] nytai edited a comment on pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
nytai edited a comment on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644981761
Merging this as is. Follow up improvements are welcome, but this issue is currently causing failures in production for us, so keeping the scope to reverting breaking changes introduced in #9939
----------------------------------------------------------------
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 pull request #10070: fix: use custom int parsing over flask int parsing in sqllab queries endpoint
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10070:
URL: https://github.com/apache/incubator-superset/pull/10070#issuecomment-644919341
@nytai I was lead to believe that [/queries/](https://github.com/apache/incubator-superset/blob/master/superset-frontend/src/SqlLab/components/QueryAutoRefresh.jsx#L71) was always sending an integer, i.e., the timestamp rounded to the nearest millisecond. Would an alternative solution be to update the frontend logic to ensure that it sends an integer? Personally I find that cleaner especially as the backend throws away the fractional millisecond component.
----------------------------------------------------------------
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