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++] JsonExtensionType equality check ignores storage type #44214

Closed
rok opened this issue Sep 24, 2024 · 1 comment · Fixed by #44215
Closed

[C++] JsonExtensionType equality check ignores storage type #44214

rok opened this issue Sep 24, 2024 · 1 comment · Fixed by #44215
Assignees
@rok
Labels
Component: C++ Priority: Blocker Marks a blocker for the release Type: bug
Milestone
18.0.0

Comments

@rok
Copy link
Member

rok commented Sep 24, 2024

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

As noted here: #13901 (comment)

Component(s)

C++
@rok rok added the Type: bug label Sep 24, 2024
@rok rok linked a pull request Sep 24, 2024 that will close this issue
GH-44214: [C++] JsonExtensionType equality check ignores storage type #44215
Merged
@github-actions github-actions bot mentioned this issue Sep 24, 2024
Merged
@rok rok mentioned this issue Sep 24, 2024
Merged
@jorisvandenbossche jorisvandenbossche added this to the 18.0.0 milestone Sep 30, 2024
@rok rok modified the milestone: 18.0.0 Oct 2, 2024
@jorisvandenbossche jorisvandenbossche added the Priority: Blocker Marks a blocker for the release label Oct 2, 2024
pitrou added a commit that referenced this issue Oct 8, 2024
@rok @pitrou
GH-44214: [C++] JsonExtensionType equality check ignores storage type (…
64891d1 
…#44215)

### Rationale for this change

As noted in #13901 (review):
```cpp
bool JsonExtensionType::ExtensionEquals(const ExtensionType& other) const {
  return other.extension_name() == this->extension_name();
}
```
> This equality check does not take into account the storage type, but only the name.
> As a consequence, a JsonExtensionType<string> type will be seen as equal to JsonExtensionType<large_string>.

### What changes are included in this PR?

This change introduces storage equality check into `JsonExtensionType` equality check.

This also fixes a storage type check in `JsonExtensionType::Make`.

### Are these changes tested?

Yes.

### Are there any user-facing changes?

No.
* GitHub Issue: #44214

Lead-authored-by: Rok Mihevc <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@pitrou
Copy link
Member

pitrou commented Oct 8, 2024

Issue resolved by pull request 44215
#44215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rok rok

Labels
Component: C++ Priority: Blocker Marks a blocker for the release Type: bug
Projects
None yet
Milestone
18.0.0
Development

Successfully merging a pull request may close this issue.

GH-44214: [C++] JsonExtensionType equality check ignores storage type rok/arrow
3 participants
@rok @jorisvandenbossche @pitrou