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.

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

[SPARK-46261][CONNECT] DataFrame.withColumnsRenamed should keep the dict/map ordering #44231

Closed

Conversation

zhengruifeng
Copy link
Contributor

What changes were proposed in this pull request?

this is a follow up of #44177

Why are the changes needed?

according to this:

Wire format ordering and map iteration ordering of map values are undefined, so you cannot rely on your map items being in a particular order.

we should not use map in protobufs when the ordering is sensitive

Does this PR introduce any user-facing change?

yes, enabled test

How was this patch tested?

enabled UT

Was this patch authored or co-authored using generative AI tooling?

no

@zhengruifeng zhengruifeng changed the title [SPARK-46260][CONNECT] DataFrame.withColumnsRenamed should keep the dict/map ordering [SPARK-46261][CONNECT] DataFrame.withColumnsRenamed should keep the dict/map ordering Dec 7, 2023
empty
@zhengruifeng
Copy link
Contributor Author

cc @HyukjinKwon @hvanhovell

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you for this synced behavior.

@dongjoon-hyun
Copy link
Member

Merged to master for Apache Spark 4.

@zhengruifeng zhengruifeng deleted the connect_with_cols_rm branch December 7, 2023 23:53
dbatomic pushed a commit to dbatomic/spark that referenced this pull request Dec 11, 2023
… dict/map ordering

### What changes were proposed in this pull request?
this is a follow up of apache#44177

### Why are the changes needed?
according to [this](https://protobuf.dev/programming-guides/proto3/#maps-features):
> Wire format ordering and map iteration ordering of map values are undefined, so you cannot rely on your map items being in a particular order.

we should not use `map` in protobufs when the ordering is sensitive

### Does this PR introduce _any_ user-facing change?
yes, enabled test

### How was this patch tested?
enabled UT

### Was this patch authored or co-authored using generative AI tooling?
no

Closes apache#44231 from zhengruifeng/connect_with_cols_rm.

Authored-by: Ruifeng Zheng <ruifengz@apache.org>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants