FIX-#7250: Revert "PERF-#6666: Avoid internal reset_index for left merge" #7251
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
anmyachev
commented
May 10, 2024
Comment on lines
-233
to
-234
| np.random.uniform(0, 100, size=(2**6, 2**6)), | ||
| np.random.uniform(0, 100, size=(2**7, 2**6)), |
There was a problem hiding this comment.
The problem is that the left merge can change the shape of the result if the merge occurs using values that are in both the left and right operands. However, in our testing we used float numbers, which were not considered identical when compared and the problem was not visible.
anmyachev
force-pushed
the
issue7250
branch
2 times, most recently
from
de95c5c
to
acd7ed2
Compare
…al reset_index for left merge" Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
anmyachev
requested review from
devin-petersohn,
mvashishtha,
RehanSD,
YarShev,
vnlitvinov,
dchigarev and
a team
as code owners
7 tasks
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
anmyachev
requested review from
aregm,
gshimansky,
ienkovich,
Garra1980,
AndreyPavlenko and
a team
as code owners
anmyachev
commented
May 12, 2024
| @@ -1994,8 +1994,6 @@ def sort_rows(self, columns, ascending, ignore_index, na_position): | |||
| drop_index_cols_after = [ | |||
| col for col in base._index_cols if col in columns | |||
| ] | |||
| if not drop_index_cols_after: | |||
| drop_index_cols_after = None | |||
There was a problem hiding this comment.
This variable cannot be None, since further on in the code we are trying to iterate over it.
anmyachev
commented
May 12, 2024
| @@ -296,7 +298,9 @@ def test_merge(test_data, test_data2): | |||
| right_on="key", | |||
| sort=sorts[j], | |||
| ) | |||
| sort_if_range_partitioning(modin_result, pandas_result) | |||
| sort_if_range_partitioning( | |||
| modin_result, pandas_result, force=StorageFormat.get() == "Hdk" | |||
There was a problem hiding this comment.
Looks like HDK doesn't preserve the same order as pandas. I don’t create an issue for this, since the engine is deprecated.
There was a problem hiding this comment.
Just to note, Modin on Ray/Dask/MPI may also have mismatched order - #2246.
YarShev
approved these changes
May 13, 2024
7 tasks
7 tasks
7 tasks
7 tasks
7 tasks
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?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -smergefailed with message:ValueError: Length mismatch: Expected axis has 36 elements, new values have 32 elements#7250docs/development/architecture.rstis up-to-date