You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "vuongtlt13 (via GitHub)" <gi...@apache.org> on 2024/07/11 07:03:25 UTC
[PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
vuongtlt13 opened a new pull request, #29557:
URL: https://github.com/apache/superset/pull/29557
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
<h2>Logic to check is_read_only_query too slow</h2>
Line: [superset/db_engine_specs/base.py](https://github.com/vuongtlt13/superset/blob/feature/improve_parsed_query/superset/db_engine_specs/base.py#L2027)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
First, I have a sql statement with length is 17121. I need check **is_read_only_query**
As you can see:
- **For current ParsedQuery class**:
Each function in **is_select**, **is_explain**, **is_show** will format sql statement by calling **sqlparse.format** function. But it takes quite long (~**0.332s** for one call). So you need to call **3 times** for checking **is_read_only_query** --> Total time: **0.9996**s
- **For new ParsedQuery class**:
You need to call only **1 time** for checking **is_read_only_query** --> Total time: **0.3278**s
<h4> ---> Reduce 66% in total time for checking is_read_only_query </4>
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually 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:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida closed pull request #29557: feat(ParsedQuery): Improve peformance when check is_read_only_query
URL: https://github.com/apache/superset/pull/29557
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2223357033
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/29557?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 64.83%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`d0ee8f9`)](https://app.codecov.io/gh/apache/superset/commit/d0ee8f9f3a891a6c5db0f27d512a51302bbf7bef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 440 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #29557 +/- ##
==========================================
+ Coverage 60.48% 64.83% +4.34%
==========================================
Files 1931 524 -1407
Lines 76236 37735 -38501
Branches 8568 0 -8568
==========================================
- Hits 46114 24466 -21648
+ Misses 28017 13269 -14748
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/29557/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/29557/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.12% <100.00%> (-0.05%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/29557/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/29557/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.71% <90.90%> (-0.10%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/29557/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.83% <100.00%> (+1.34%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/29557/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.68% <100.00%> (+2.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/29557?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2223365472
Tagging @betodealmeida since he's been working on deprecating `sqlparse` in favor of `sqlglot` -> https://github.com/apache/superset/issues/26786
The PR looks ok though it'd be tempting to refactor further, maybe using `@property` decorator and associated/corresponding private vars, as in
```python
@property
def my_property(self):
if self._my_property is None
self._my_property = ...
return self._my_property
```
300ms is still pretty ridiculous to check the first word of a string, tempting to aim at sub-50ms and more refactoring if you're up for it.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2803119826
Closing/reopening to get the current state of CI here. Then hopefully @betodealmeida can give it a review.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "vuongtlt13 (via GitHub)" <gi...@apache.org>.
vuongtlt13 closed pull request #29557: feat(ParsedQuery): Improve peformance when check is_read_only_query
URL: https://github.com/apache/superset/pull/29557
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2992169221
No longer applicable.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "vuongtlt13 (via GitHub)" <gi...@apache.org>.
vuongtlt13 commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2826273954
> Looks like this is in need of some CI help:
>
> Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
>
> ```
> pip3 install -r requirements/development.txt
> pre-commit install
> A series of checks will now run when you make a git commit.
> ```
>
> Alternatively it is possible to run pre-commit by running pre-commit manually:
>
> ```
> pre-commit run --all-files
> ```
Tks, I updated
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2803174349
Looks like this is in need of some CI help:
Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
```
pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.
```
Alternatively it is possible to run pre-commit by running pre-commit manually:
```
pre-commit run --all-files
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "vuongtlt13 (via GitHub)" <gi...@apache.org>.
vuongtlt13 commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2224347209
> Wow this takes much more time than I would have thought intuitively. Was the SQL query you tested with very large?
Yes, my SQL query has 17121 characters in length
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #29557: feat(ParsedQuery): Improve peformance when check is_read_only_query
URL: https://github.com/apache/superset/pull/29557
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2839381018
@betodealmeida
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2223347943
Wow this takes much more time than I would have thought intuitively. Was the SQL query you tested with very large?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "vuongtlt13 (via GitHub)" <gi...@apache.org>.
vuongtlt13 commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2230427712
@michael-s-molina can you give me approval for my PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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
Re: [PR] feat(ParsedQuery): Improve peformance when check is_read_only_query [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #29557:
URL: https://github.com/apache/superset/pull/29557#issuecomment-2230671280
> @michael-s-molina can you give me approval for my PR?
As @mistercrunch mentioned above, @betodealmeida is the one working on replacing `sqlparse` with `sqlglot` so he's the best person to review this PR. The result of his work might even delete the changed file, so I'll let him review the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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