FEAT-#4320: Add connectorx as an alternative engine for read_sql
#4346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Codecov Report
@@ Coverage Diff @@
## master #4346 +/- ##
===========================================
- Coverage 87.23% 60.20% -27.04%
===========================================
Files 219 219
Lines 17927 17945 +18
===========================================
- Hits 15639 10804 -4835
- Misses 2288 7141 +4853
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
|
There was a problem hiding this comment.
Looks great! A couple of requests:
- If
ReadSqlEngineisconnectorx, butconnectorxis not installed, we will get some errors. Can we check that connectorx is installed in the logic forread_sql, the warn the user and resetReadSqlEnginetopandas? - Please also enable CI tests and add
connectorxto the test_requires file.
|
Thanks for the review!
Modified at here, can you take a look?
Can you give me some suggestions on how to enable CI? I modified the original Also I didn't find the |
|
@wangxiaoying We are dealing with some CI environment issues, working on fix in #4364 |
Got it! Please let me know if you need any actions from my side. |
|
@wangxiaoying I think it's all set, if you can rebase on the master branch, CI should pass! |
8eb1a92
to
d90e77f
Compare
YarShev
reviewed
Apr 6, 2022
|
Hi @YarShev , thanks for the suggestions. I tried to test on the capital letter solution but it still has the same issue. I have added two debug messages in the latest commit. As the screenshot below, the value found in SqlDispatcher is
I set the |
|
@wangxiaoying, the issue is that parse method is run in worker processes, which do not see an updated |
YarShev
reviewed
Apr 7, 2022
|
@YarShev , I have updated the code following your suggestions. And the issue is resolved after passing the engine configuration to the parser! |
e8b7971
to
1a54f5c
Compare
YarShev
reviewed
Apr 8, 2022
6c16413
to
533b252
Compare
|
@YarShev , I have fixed the comments. Please let me know if you need other actions from my side! |
|
@wangxiaoying, I've run CI and lint (commit) job failed. To pass it you should format the commit message as follows: git commit --amendin an opened up editor you should put the lines in the following order after doing that push the changes git push -f |
YarShev
reviewed
Apr 10, 2022
0a1f69d
to
ae39c1b
Compare
|
@YarShev , I've updated the commit message and applied those code suggestions. Can you take a look again? |
|
Some failures have occurred in CI. I restarted tests to see if it helps. |
|
@wangxiaoying it looks like a SQL syntax error occurred in CI: https://github.com/modin-project/modin/runs/5988352034?check_suite_focus=true |
|
@devin-petersohn indeed, the reason is that we remove the clauses such as |
ae39c1b
to
8c1c34a
Compare
|
@devin-petersohn @YarShev , we have released an alpha version for the bug fix and requirement files are updated. Can you please try a gain? |
|
I ran tests. Will see how it goes. |
8c1c34a
to
f1040f5
Compare
|
@YarShev Thanks for the example. I have updated the release note. I added to "Performance enhancements" but let me know if you think other sections suit this issue better : ) |
|
I think it is okay. Please fix conflicts. |
…r `read_sql` Co-authored-by: Devin Petersohn <[email protected]> Co-authored-by: Yaroslav Igoshev <[email protected]> Signed-off-by: wangxiaoying <[email protected]>
f1040f5
to
a645063
Compare
@YarShev Done |
YarShev
approved these changes
Apr 14, 2022
There was a problem hiding this comment.
@wangxiaoying, LGTM, thanks! @devin-petersohn, any other comments?
devin-petersohn
approved these changes
Apr 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What do these changes do?
Add connectorx as an alternative engine for
read_sql. #4320I tested locally by setting the env var
READ_SQL_ENGINEtoconnectorxand it works fine. However, when I set the config byReadSqlEngine.put("connectorx")(like here), the result ofReadSqlEngineretrieved in thePandasSQLParserispandas. It's very likely that I missed some setup of the config. Can you take a look?flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/development/architecture.rstis up-to-date