FIX-#7329: Do not sort columns on df.update #7330
Conversation
Signed-off-by: Igoshev, Iaroslav <[email protected]>
YarShev
requested review from
devin-petersohn,
mvashishtha,
RehanSD,
vnlitvinov,
anmyachev,
dchigarev and
a team
as code owners
| @@ -3886,7 +3886,7 @@ def n_ary_op( | |||
| 1, | |||
| new_right_frames, | |||
| join_type, | |||
| sort=True, | |||
There was a problem hiding this comment.
Why was sort=True here?
There was a problem hiding this comment.
I think we need to add more tests because you are changing the behavior of a low-level function that is used to implement many high-level functions.
There was a problem hiding this comment.
My assumption is that someone was thinking this should work for all cases.
There was a problem hiding this comment.
What tests would you suggest to add?
There was a problem hiding this comment.
We already have pretty extensive test suite for checking high-level functions.
There was a problem hiding this comment.
My assumption is that someone was thinking this should work for all cases.
Why do you think that this should always work without sorting now? My doubts, for example, are based on this line:
What tests would you suggest to add?
I don't have any specific functions, you can add for any few you find.
We already have pretty extensive test suite for checking high-level functions.
I'm not sure we wrote many tests taking into account the order of the columns.
There was a problem hiding this comment.
Indeed, handling sort here is a bit tricky. Changed the logic and added a test for some bin op.
Signed-off-by: Igoshev, Iaroslav <[email protected]>
| sort=True, | ||
| sort=( | ||
| not all( | ||
| self.get_axis(1).equals(right.get_axis(1)) |
There was a problem hiding this comment.
would it make sense to move this logic to def _copartition and do this there if sort is None?
Signed-off-by: Igoshev, Iaroslav <[email protected]>
What do these changes do?
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