Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FEAT-#7254: Support right merge/join #7226

Merged
merged 22 commits into from May 13, 2024
Merged

FEAT-#7254: Support right merge/join #7226

merged 22 commits into from May 13, 2024

Conversation

anmyachev
Copy link
Collaborator

@anmyachev anmyachev commented Apr 26, 2024 •
edited

What do these changes do?

Blocked on #7251

Performance: 1.7 sec (the PR) vs 3.1 sec (on main) on Ray (8 cores)

import modin.pandas as pd
from time import time
import numpy as np
from modin.utils import execute

np.random.seed(42)

df1 = pd.DataFrame(np.random.randint(0, 100_000,size=(400_000, 64)))
df2 = pd.DataFrame(np.random.randint(0, 100_000,size=(400_000, 64)))
execute(df1)
execute(df2)


for _ in range(3):
    start = time()
    result = pd.merge(df1, df2, how="right", on=1)
    execute(result)
    print(f"{result.shape=}")
    print(f"merge time: {time()-start}")
  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves Support right merge/join #7254
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date

@anmyachev
PERF-#0000: try merge optimization
e261c83 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
just a try
649fb0d 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
fix
e089cd6 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
try to implement right merge
b380dfd 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
move import from typing_extensions
774e9e4 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
FIX-modin-project#7250: Revert "PERF-modin-project#6666: Avoid intern…
3692720 
…al reset_index for left merge"

Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
Merge branch 'issue7250' into merge-swap-operand
49577eb
@anmyachev
fix after merge
48942fd 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
try fix HDK errors
d60e0cf 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
fix
eb65291 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
fix
998d841 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
try to fix HDK
d72ed82 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
Merge branch 'issue7250' into merge-swap-operand
2194844
@anmyachev
fixes + implementation for right join
089bcce 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
fix HDK errors
6cce47b 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev anmyachev changed the title PERF-#0000: try merge optimization FEAT-#7254: Support right merge/join May 13, 2024
@anmyachev anmyachev marked this pull request as ready for review May 13, 2024 00:33
@anmyachev anmyachev requested review from mvashishtha, RehanSD and a team as code owners May 13, 2024 00:33
@anmyachev
fix
4a911d0 
Signed-off-by: Anatoly Myachev <[email protected]>
AndreyPavlenko
AndreyPavlenko previously approved these changes May 13, 2024
View reviewed changes
@anmyachev
Copy link
Collaborator Author

@YarShev ready for review

YarShev
YarShev reviewed May 13, 2024
View reviewed changes
modin/tests/pandas/dataframe/test_join_sort.py Outdated Show resolved Hide resolved
modin/tests/pandas/dataframe/test_join_sort.py Outdated Show resolved Hide resolved
modin/tests/pandas/dataframe/test_join_sort.py
@@ -140,7 +141,11 @@ def test_join(test_data, test_data2):
lsuffix="_caller",
rsuffix="_other",
)
df_equals(modin_result, pandas_result)
if sorts[j]:
# sorting in `join` is implemented through range partitioning technique
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand this comment. sorting is just called in df_equals_and_sort. Should we remove this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expanded the comment a little

modin/experimental/core/execution/native/implementations/hdk_on_native/dataframe/dataframe.py Outdated Show resolved Hide resolved
modin/core/storage_formats/pandas/query_compiler.py
def map_func(left, right, kwargs=kwargs): # pragma: no cover
return pandas.DataFrame.join(left, right, **kwargs)
if how in ["left", "inner"] or (
how == "right" and right._modin_frame._partitions.size != 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if left has size equals to 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty dataframes are processed at a higher level (simply defaulted to pandas) and all implementation logic relies on the fact that the left operand has a non-empty set of partitions. Therefore, to avoid an error, we need to handle this situation as before.

modin/core/storage_formats/pandas/merge.py
if (
(
how in ["left", "inner"]
or (how == "right" and right._modin_frame._partitions.size != 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7226 (comment)

@anmyachev
address review comments
7d7914b 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
preserve dtypes for empty dataframes
981c74e 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
make comparison stable after merge operation
b940a3b 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev
fix
0b30e13 
Signed-off-by: Anatoly Myachev <[email protected]>
YarShev
YarShev reviewed May 13, 2024
View reviewed changes
modin/tests/pandas/dataframe/test_join_sort.py Outdated Show resolved Hide resolved
modin/tests/pandas/dataframe/test_join_sort.py
data = np.random.randint(0, 100, size=(64, 64))
eval_general(
*create_test_dfs(data),
lambda df: df.join(df.iloc[:0], on=1, how=how, lsuffix="_caller"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not use on parameter for a similar test with merge?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Different threads of execution, for merge function we can omit this parameter so that a certain part of the code starts executing.

modin/core/dataframe/pandas/partitioning/partition_manager.py Outdated Show resolved Hide resolved
@anmyachev @YarShev
Apply suggestions from code review
3090e09 
Co-authored-by: Iaroslav Igoshev <[email protected]>
@anmyachev anmyachev merged commit 1c0d9a6 into modin-project:main May 13, 2024
38 checks passed
@anmyachev anmyachev deleted the merge-swap-operand branch May 13, 2024 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YarShev YarShev YarShev approved these changes

@AndreyPavlenko AndreyPavlenko AndreyPavlenko left review comments

@aregm aregm Awaiting requested review from aregm

@gshimansky gshimansky Awaiting requested review from gshimansky

@ienkovich ienkovich Awaiting requested review from ienkovich

@Garra1980 Garra1980 Awaiting requested review from Garra1980

@vnlitvinov vnlitvinov Awaiting requested review from vnlitvinov vnlitvinov is a code owner

@dchigarev dchigarev Awaiting requested review from dchigarev dchigarev is a code owner

@devin-petersohn devin-petersohn Awaiting requested review from devin-petersohn devin-petersohn is a code owner

@mvashishtha mvashishtha Awaiting requested review from mvashishtha mvashishtha is a code owner

@RehanSD RehanSD Awaiting requested review from RehanSD RehanSD is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support right merge/join
3 participants
@anmyachev @AndreyPavlenko @YarShev