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

[C++] C data bridge doesn't preserve map values nullability on import #34983

Closed
wjones127 opened this issue Apr 8, 2023 · 0 comments · Fixed by #35013
Closed

[C++] C data bridge doesn't preserve map values nullability on import #34983

wjones127 opened this issue Apr 8, 2023 · 0 comments · Fixed by #35013
Assignees
@wjones127
Labels
Component: C++ Type: bug
Milestone
12.0.0

Comments

@wjones127
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

The nullability of the values field is lost here, since we pass as a type and not a field:

arrow/cpp/src/arrow/c/bridge.cc

Line 1105 in 82d48a9

type_ = map(value_type->field(0)->type(), value_type->field(1)->type(), keys_sorted);

Component(s)

C++
@wjones127 wjones127 self-assigned this Apr 8, 2023
@github-actions github-actions bot mentioned this issue Apr 10, 2023
Merged
wjones127 added a commit that referenced this issue Apr 11, 2023
@wjones127
GH-34983: [C++] Preserve map values nullability on C Data Interface i…
c7ba0cd 
…mport (#35013)

### Rationale for this change

Map types with non-nullable values don't roundtrip right now through C Data interface, when they arguably should.

I also found that field names don't roundtrip, but I think that might be a feature, not a bug. Therefore I added unit tests enforcing it for now.
 
### What changes are included in this PR?

### Are these changes tested?

Yes, added various unit tests for export and roundtrip.

### Are there any user-facing changes?

* Closes: #34983

Authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Will Jones <willjones127@gmail.com>
@wjones127 wjones127 added this to the 12.0.0 milestone Apr 11, 2023
liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this issue May 11, 2023
@wjones127 @liujiacheng777
apacheGH-34983: [C++] Preserve map values nullability on C Data Inter…
5d3af94 
…face import (apache#35013)

### Rationale for this change

Map types with non-nullable values don't roundtrip right now through C Data interface, when they arguably should.

I also found that field names don't roundtrip, but I think that might be a feature, not a bug. Therefore I added unit tests enforcing it for now.
 
### What changes are included in this PR?

### Are these changes tested?

Yes, added various unit tests for export and roundtrip.

### Are there any user-facing changes?

* Closes: apache#34983

Authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Will Jones <willjones127@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this issue May 15, 2023
@wjones127 @ArgusLi
apacheGH-34983: [C++] Preserve map values nullability on C Data Inter…
478baa6 
…face import (apache#35013)

### Rationale for this change

Map types with non-nullable values don't roundtrip right now through C Data interface, when they arguably should.

I also found that field names don't roundtrip, but I think that might be a feature, not a bug. Therefore I added unit tests enforcing it for now.
 
### What changes are included in this PR?

### Are these changes tested?

Yes, added various unit tests for export and roundtrip.

### Are there any user-facing changes?

* Closes: apache#34983

Authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Will Jones <willjones127@gmail.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this issue May 16, 2023
@wjones127 @rtpsw
apacheGH-34983: [C++] Preserve map values nullability on C Data Inter…
c373bda 
…face import (apache#35013)

### Rationale for this change

Map types with non-nullable values don't roundtrip right now through C Data interface, when they arguably should.

I also found that field names don't roundtrip, but I think that might be a feature, not a bug. Therefore I added unit tests enforcing it for now.
 
### What changes are included in this PR?

### Are these changes tested?

Yes, added various unit tests for export and roundtrip.

### Are there any user-facing changes?

* Closes: apache#34983

Authored-by: Will Jones <willjones127@gmail.com>
Signed-off-by: Will Jones <willjones127@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wjones127 wjones127

Labels
Component: C++ Type: bug
Projects
None yet
Milestone
12.0.0
Development

Successfully merging a pull request may close this issue.

GH-34983: [C++] Preserve map values nullability on C Data Interface import wjones127/arrow
1 participant
@wjones127