REFACTOR-#1763: Move logic of merge into the query compiler
#1764
Conversation
This file contains hidden or 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
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
from
f8aef29 to
94efa85
Compare
Codecov Report
@@ Coverage Diff @@
## master #1764 +/- ##
==========================================
+ Coverage 84.23% 84.25% +0.02%
==========================================
Files 77 77
Lines 8056 8062 +6
==========================================
+ Hits 6786 6793 +7
+ Misses 1270 1269 -1
Continue to review full report at Codecov.
|
|
|
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
from
94efa85 to
9905b60
Compare
|
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
2 times, most recently
from
66c2469 to
2dc1142
Compare
Collaborator
Author
|
@devin-petersohn , just a friendly reminder to review. |
Collaborator
|
@YarShev As described, you made 3 independent changes in one PR. Could you split this one into several? |
Collaborator
Author
|
@anmyachev , I don't think that these changes are so big and confused in order to split them into several PRs. If you have any questions on changes, I can explain. |
Collaborator
Indeed, there are few changes, but nevertheless, it is not worth mixing fundamental tasks such as refactoring and bug fixing. So there is minimum 2 PRs. |
Collaborator
Author
|
The changes which are in the PR are dependent on each other. My description is just an explanation for improving of review. I can create an issue for each unit which I mentioned. But let's imagine, how many issues are needed for such changes like these #1638. So, I think it is just a wasting of time. |
Collaborator
This will be a challenge for #1638, but it will not be a challenge for your PR. |
Collaborator
Author
|
#1638 is a corner case. Still, I believe that can be resolved without dividing into several PRs. |
YarShev
changed the title
merge into the query compilermerge into the query compiler
devin-petersohn
requested changes
Jul 22, 2020
Collaborator
devin-petersohn
left a comment
devin-petersohn
left a comment
There was a problem hiding this comment.
Looks great @YarShev, a couple of minor nit formatting changes.
YarShev
changed the title
merge into the query compilermerge into the query compiler
YarShev
changed the title
merge into the query compilermerge into the query compiler
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
3 times, most recently
from
7b70694 to
75946ab
Compare
YarShev
changed the title
merge into the query compilermerge into the query compiler
YarShev
changed the title
merge into the query compilermerge into the query compiler
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
from
75946ab to
835cf3f
Compare
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
9 times, most recently
from
6c283a7 to
708d50f
Compare
Collaborator
Author
|
@devin-petersohn , just a friendly reminder to review. |
Collaborator
Author
|
@devin-petersohn , I have increased the size of commit message header because I think, |
gshimansky
requested changes
Jul 23, 2020
Collaborator
gshimansky
left a comment
gshimansky
left a comment
There was a problem hiding this comment.
I have a small question, but my main concern is that there is no new test to verify that #1771 is fixed. We may have more regressions if we don't add tests for every bug.
Collaborator
There was a problem hiding this comment.
not sort if sort else sort is always False. Is it what is meant here?
Collaborator
Author
There was a problem hiding this comment.
Yes, we don't want to sort cells within execution of function on partitions in order to avoid additional overhead. The logic regarding sorting is below.
Collaborator
There was a problem hiding this comment.
Then should this line be replaced with kwargs["sort"] = False?
Collaborator
Author
There was a problem hiding this comment.
Yes, it makes sense, done. Thanks!
Collaborator
Author
|
@gshimansky , Regarding to the fix for #1771 . There is a test for this. Here when |
Collaborator
I didn't quite understand how #1771 bug is tested. Right now on master branch all tests pass while the bug exists. We need a test that would fail on current master branch to verify that this PR fixes this bug. If you say that existing tests are enough to test for this bug and require just a small change, can you add this change to this PR? |
Collaborator
Author
|
@gshimansky , These changes are in this PR. It is the first one and the second one. The bug was that: if we call |
Collaborator
Yes I saw the bug fix in this PR. I didn't see any new test in PR. None of test files are modified in this PR. There is a general rule that when you fix a bug you add a test for it. This test should fail without this fix and pass with it. |
commitlint.config.js
Outdated
Collaborator
There was a problem hiding this comment.
Please open a new issue and new PR for this change.
into the query compiler Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>
YarShev
force-pushed
the
dev/yigoshev-refactor-merge
branch
from
708d50f to
19c5109
Compare
Collaborator
Author
|
@gshimansky , I added an appropriate test for #1771 . |
gshimansky
approved these changes
Jul 23, 2020
devin-petersohn
approved these changes
Jul 23, 2020
Collaborator
|
@YarShev After CI passes I will merge this. Thanks! |
aregm
pushed a commit
to aregm/modin
that referenced
this pull request
Sep 16, 2020
into the query compiler Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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?
These changes move out the logic of
mergeandsort_indexfrom API layer into the query compiler, fix incorrect computing ofrow_lengthsandcolumn_widthsas well.flake8 modinblack --check modingit commit -smergeinto the query compiler #1763, merge throws exception on MODIN_ENGINE=python #1771