modin
modin copied to clipboard
modin-project
FEAT-#4946: Replace OmniSci with HDK
What do these changes do?
OmniSci has been replaced in the code, paths and documentation.
Simple renaming is not sufficient for the documentation. An
additional work is required to review and rewrite the documentation.
- [x] commit message follows format outlined here
- [x] passes
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py - [x] passes
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py - [x] signed commit with
git commit -s - [x] Resolves #4946
- [ ] tests added and passing
- [ ] module layout described at
docs/development/architecture.rstis up-to-date - [ ] added (Issue Number: PR title (PR Number)) and github username to release notes for next major release
Codecov Report
Merging #4947 (e89c521) into master (d6d503a) will increase coverage by
4.69%. The diff coverage is88.70%.
@@ Coverage Diff @@
## master #4947 +/- ##
==========================================
+ Coverage 84.91% 89.60% +4.69%
==========================================
Files 267 267
Lines 19744 20044 +300
==========================================
+ Hits 16766 17961 +1195
+ Misses 2978 2083 -895
| Impacted Files | Coverage Δ | |
|---|---|---|
| modin/experimental/cloud/hdk.py | 0.00% <0.00%> (ø) |
|
| ...e/implementations/hdk_on_native/calcite_builder.py | 96.36% <ø> (ø) |
|
| ...mplementations/hdk_on_native/calcite_serializer.py | 98.70% <ø> (ø) |
|
| ...native/implementations/hdk_on_native/df_algebra.py | 76.88% <ø> (ø) |
|
| ...ution/native/implementations/hdk_on_native/expr.py | 85.63% <ø> (ø) |
|
| ...imental/core/storage_formats/hdk/query_compiler.py | 94.49% <ø> (ø) |
|
| modin/experimental/cloud/cluster.py | 79.20% <50.00%> (+20.79%) |
:arrow_up: |
| modin/pandas/__init__.py | 71.26% <53.84%> (+4.59%) |
:arrow_up: |
| .../native/implementations/hdk_on_native/db_worker.py | 87.50% <66.66%> (ø) |
|
| modin/utils.py | 93.66% <66.66%> (+14.02%) |
:arrow_up: |
| ... and 66 more |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request fixes 2 alerts when merging 6f77cf2eb6728beafcfdb02a0b8d95fcd2f56da3 into a6f47c8e1c27d85fc09926bb35c2f1a65a6d3e79 - view on LGTM.com
fixed alerts:
- 2 for Unused import
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request fixes 2 alerts when merging a14162a4c22bc4fb13deb5b81fdc7820f052af51 into a6f47c8e1c27d85fc09926bb35c2f1a65a6d3e79 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging 4308319125347108f874b7ff2547c028f7110957 into a6f47c8e1c27d85fc09926bb35c2f1a65a6d3e79 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging bd2c0ebf876b274163d2b8ab45b645fed9a95296 into b09dd424e95830e6a19a631741eb8381b9020ee5 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging 04b3947a172c6cb94472d8e4387da23286126c78 into af6f8277cf9caeb7dec8b390ef3652ec0fa04774 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging fc9e0241a272048fdf34e89c37686a0c92ffa45e into af6f8277cf9caeb7dec8b390ef3652ec0fa04774 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging e3fac0d595459f02903c8ccc694362040abcc79d into 7c260ad55d40bd31de27a21c66f47202e430b66e - view on LGTM.com
fixed alerts:
- 2 for Unused import
@AndreyPavlenko, please ping me when comments are addressed.
Do you mean doc_checker.py? Comments added.
No, I mean when all comments are addressed and the PR is ready for review again.
No, I mean when all comments are addressed and the PR is ready for review again.
Yes, I've rebased the commit accordingly.
@AndreyPavlenko, please apply all suggestions I left (even those which are in the hidden conversation, just roll out it to see). If something is not applicable, please resolve those and put a comment like "not applicable".
Aside: It would be better to address comments in separate commits to make it easier for reviewers inspect changes.
@AndreyPavlenko please note that "refactoring" is changing some code implementation without changing any outside behaviour. What you've done here should be labelled FEAT as it's a feature change.
Aside: It would be better to address comments in separate commits to make it easier for reviewers inspect changes.
Should I do multiple commits (one per comment) and then squash the commits before the merge?
Aside: It would be better to address comments in separate commits to make it easier for reviewers inspect changes.
Should I do multiple commits (one per comment) and then squash the commits before the merge?
You can do as much commits as you want. The reviewer who is going to merge the changes will squash the commits. We require only the first commit to follow the contributing guideline. Btw, you can apply all suggestions in one commit as batch, GitHub allows to do that.
This pull request fixes 2 alerts when merging 939693db4f4e10a47d609171eafc979e491e1eab into c10f0f9902b3d4e66135e83773b7ff2aa3b4215d - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging 2a9607b0a477e9c2dc53d493bcbd086a5a77cdfc into c10f0f9902b3d4e66135e83773b7ff2aa3b4215d - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging b5e7dec4d061f63568cf38ac8c1c9c9f66effee0 into 45879a6c42506deed2342de7f3dbf0ff2b223d1a - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging a896407b2d072a5ed3bdeae0d2185218f28d1399 into 45879a6c42506deed2342de7f3dbf0ff2b223d1a - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging 1c4d19e08c0e8146aa6e6122ace1c2bdbd6ebb0b into d6d503ac7c3028d871c34d9e99e925ddb0746df6 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging e6862829d4aac6e95c4fa65073872f6b597e057b into d6d503ac7c3028d871c34d9e99e925ddb0746df6 - view on LGTM.com
fixed alerts:
- 2 for Unused import
This pull request fixes 2 alerts when merging e89c521a12fb67e434d76ceb28d7cf70c3f4f12a into d6d503ac7c3028d871c34d9e99e925ddb0746df6 - view on LGTM.com
fixed alerts:
- 2 for Unused import