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

[C++] ABI break in patch release 15.0.1 #40604

Closed
musicinmybrain opened this issue Mar 16, 2024 · 8 comments
Closed

[C++] ABI break in patch release 15.0.1 #40604

musicinmybrain opened this issue Mar 16, 2024 · 8 comments
Labels
Breaking Change Includes a breaking change to the API Component: C++ Type: bug

Comments

@musicinmybrain
Copy link

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

An ABI break was reported between 15.0.0 and 15.0.1 in Fedora Linux (downstream bug).

It looks like 91be098 changed the types of several class method parameters that appear in the public API from const std::string& to std::string_view.

================ changes of 'libarrow.so.1500.0.0'===============
  Functions changes summary: 6 Removed, 0 Changed (11 filtered out), 6 Added functions
  Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

  6 Removed functions:

    [D] 'method bool arrow::KeyValueMetadata::Contains(const std::string&) const'    {_ZNK5arrow16KeyValueMetadata8ContainsERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE}
    [D] 'method arrow::Status arrow::KeyValueMetadata::Delete(const std::string&)'    {_ZN5arrow16KeyValueMetadata6DeleteERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE}
    [D] 'method int arrow::KeyValueMetadata::FindKey(const std::string&) const'    {_ZNK5arrow16KeyValueMetadata7FindKeyERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE}
    [D] 'method arrow::Result<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > arrow::KeyValueMetadata::Get(const std::string&) const'    {_ZNK5arrow16KeyValueMetadata3GetERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE}
    [D] 'method arrow::Status arrow::KeyValueMetadata::Set(const std::string&, const std::string&)'    {_ZN5arrow16KeyValueMetadata3SetERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES8_}
    [D] 'function arrow::Status arrow::compute::internal::FSBFilterExec(arrow::compute::KernelContext*, const arrow::compute::ExecSpan&, arrow::compute::ExecResult*)'    {_ZN5arrow7compute8internal13FSBFilterExecEPNS0_13KernelContextERKNS0_8ExecSpanEPNS0_10ExecResultE}

  6 Added functions:

    [A] 'method bool arrow::KeyValueMetadata::Contains(std::string_view) const'    {_ZNK5arrow16KeyValueMetadata8ContainsESt17basic_string_viewIcSt11char_traitsIcEE}
    [A] 'method arrow::Status arrow::KeyValueMetadata::Delete(std::string_view)'    {_ZN5arrow16KeyValueMetadata6DeleteESt17basic_string_viewIcSt11char_traitsIcEE}
    [A] 'method int arrow::KeyValueMetadata::FindKey(std::string_view) const'    {_ZNK5arrow16KeyValueMetadata7FindKeyESt17basic_string_viewIcSt11char_traitsIcEE}
    [A] 'method arrow::Result<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > arrow::KeyValueMetadata::Get(std::string_view) const'    {_ZNK5arrow16KeyValueMetadata3GetB5cxx11ESt17basic_string_viewIcSt11char_traitsIcEE}
    [A] 'method arrow::Status arrow::KeyValueMetadata::Set(std::string, std::string)'    {_ZN5arrow16KeyValueMetadata3SetENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES6_}
    [A] 'function arrow::Status arrow::compute::internal::PrimitiveTakeExec(arrow::compute::KernelContext*, const arrow::compute::ExecSpan&, arrow::compute::ExecResult*)'    {_ZN5arrow7compute8internal17PrimitiveTakeExecEPNS0_13KernelContextERKNS0_8ExecSpanEPNS0_10ExecResultE}

================ end of changes of 'libarrow.so.1500.0.0'===============

Component(s)

C++

@musicinmybrain musicinmybrain changed the title ABI break in patch release 15.0.1 [C++] ABI break in patch release 15.0.1 Mar 16, 2024
@kou
Copy link
Member

kou commented Mar 17, 2024

@pitrou Could you take a look at this? It seems that we should have kept the backward compatibility in the PR to follow the semantic versioning.

@pitrou
Copy link
Member

pitrou commented Mar 17, 2024

Ow... well, we could revert the KeyValueMetadata changes if we plan to do a new 15.0.x release.

@ianmcook
Copy link
Member

FYI @raulcd

@amoeba amoeba added the Breaking Change Includes a breaking change to the API label Mar 21, 2024
@amoeba
Copy link
Member

amoeba commented Mar 21, 2024

I labeled this as a breaking change for the moment, since it seems to me that reverting the breaking change would be a breaking change too. If this isn't true (or if we don't revert the change) feel free to remove the label and probably leave a comment.

@pitrou
Copy link
Member

pitrou commented Mar 21, 2024

Yes, reverting would also be a breaking change. Also, we're unlikely to do a 15.0.3 IMHO.

@kalebskeithley
Copy link

kalebskeithley commented Apr 22, 2024

It doesn't appear that this was "fixed", i.e. the change reverted, in 16.0.0. Would it be correct to conclude that the API change is permanent?

@pitrou
Copy link
Member

pitrou commented May 2, 2024

@kalebskeithley We don't guarantee any ABI stability across feature releases, so that is normal indeed.

@pitrou
Copy link
Member

pitrou commented Sep 18, 2024

While this breakage was unfortunate, there's nothing that can be done anymore since the latest released version is 17.0.0. I'm therefore closing this issue. Sorry for the disruption!

@pitrou pitrou closed this as not planned Won't fix, can't repro, duplicate, stale Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Includes a breaking change to the API Component: C++ Type: bug
Projects
None yet
Development

No branches or pull requests

6 participants