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++] Schema C Data Interface import keeps metadata for registered extension type #39865

Closed
pitrou opened this issue Jan 31, 2024 · 0 comments · Fixed by #39866
Closed

[C++] Schema C Data Interface import keeps metadata for registered extension type #39865

pitrou opened this issue Jan 31, 2024 · 0 comments · Fixed by #39866
Assignees
@pitrou
Labels
Breaking Change Includes a breaking change to the API Component: C++ Type: bug
Milestone
15.0.1

Comments

@pitrou
Copy link
Member

pitrou commented Jan 31, 2024

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

Originally found out by Arrow Rust: when importing a Schema with an extension type, the extension type will keep its serialized extension metadata even if there is a corresponding registered extension.

Instead, we should strip the corresponding metadata if the extension type is known.

Component(s)

C++
@pitrou pitrou self-assigned this Jan 31, 2024
@pitrou pitrou mentioned this issue Jan 31, 2024
Closed
pitrou added a commit to pitrou/arrow that referenced this issue Jan 31, 2024
@pitrou
apacheGH-39865: [C++] Strip extension metadata when importing a regis…
99c451e 
…tered extension
@github-actions github-actions bot mentioned this issue Jan 31, 2024
Merged
@atwam atwam mentioned this issue Jan 31, 2024
Merged
pitrou added a commit to pitrou/arrow that referenced this issue Feb 1, 2024
@pitrou
apacheGH-39865: [C++] Strip extension metadata when importing a regis…
3c3033d 
…tered extension
pitrou added a commit to pitrou/arrow that referenced this issue Feb 5, 2024
@pitrou
apacheGH-39865: [C++] Strip extension metadata when importing a regis…
4ca28fc 
…tered extension
pitrou added a commit that referenced this issue Feb 5, 2024
@pitrou
GH-39865: [C++] Strip extension metadata when importing a registered …
56951fe 
…extension (#39866)

### Rationale for this change

When importing an extension type from the C Data Interface and the extension type is registered, we would still leave the extension-related metadata on the storage type.

### What changes are included in this PR?

Strip extension-related metadata on the storage type if we succeed in recreating the extension type.
This matches the behavior of the IPC layer and allows for more exact roundtripping.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No, unless people mistakingly rely on the presence of said metadata.
* Closes: #39865

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou added this to the 16.0.0 milestone Feb 5, 2024
@pitrou pitrou modified the milestones: 16.0.0, 15.0.1 Feb 14, 2024
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@pitrou @dgreiss
apacheGH-39865: [C++] Strip extension metadata when importing a regis…
2e108c0 
…tered extension (apache#39866)

### Rationale for this change

When importing an extension type from the C Data Interface and the extension type is registered, we would still leave the extension-related metadata on the storage type.

### What changes are included in this PR?

Strip extension-related metadata on the storage type if we succeed in recreating the extension type.
This matches the behavior of the IPC layer and allows for more exact roundtripping.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No, unless people mistakingly rely on the presence of said metadata.
* Closes: apache#39865

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
raulcd pushed a commit that referenced this issue Feb 20, 2024
@pitrou @raulcd
GH-39865: [C++] Strip extension metadata when importing a registered …
91be098 
…extension (#39866)

### Rationale for this change

When importing an extension type from the C Data Interface and the extension type is registered, we would still leave the extension-related metadata on the storage type.

### What changes are included in this PR?

Strip extension-related metadata on the storage type if we succeed in recreating the extension type.
This matches the behavior of the IPC layer and allows for more exact roundtripping.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No, unless people mistakingly rely on the presence of said metadata.
* Closes: #39865

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@pitrou @zanmato1984
apacheGH-39865: [C++] Strip extension metadata when importing a regis…
41fa07b 
…tered extension (apache#39866)

### Rationale for this change

When importing an extension type from the C Data Interface and the extension type is registered, we would still leave the extension-related metadata on the storage type.

### What changes are included in this PR?

Strip extension-related metadata on the storage type if we succeed in recreating the extension type.
This matches the behavior of the IPC layer and allows for more exact roundtripping.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No, unless people mistakingly rely on the presence of said metadata.
* Closes: apache#39865

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@pitrou @thisisnic
apacheGH-39865: [C++] Strip extension metadata when importing a regis…
4ef5cf2 
…tered extension (apache#39866)

### Rationale for this change

When importing an extension type from the C Data Interface and the extension type is registered, we would still leave the extension-related metadata on the storage type.

### What changes are included in this PR?

Strip extension-related metadata on the storage type if we succeed in recreating the extension type.
This matches the behavior of the IPC layer and allows for more exact roundtripping.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No, unless people mistakingly rely on the presence of said metadata.
* Closes: apache#39865

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@amoeba amoeba added the Breaking Change Includes a breaking change to the API label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pitrou pitrou

Labels
Breaking Change Includes a breaking change to the API Component: C++ Type: bug
Projects
None yet
Milestone
15.0.1
Development

Successfully merging a pull request may close this issue.

GH-39865: [C++] Strip extension metadata when importing a registered extension pitrou/arrow
2 participants
@amoeba @pitrou