GH-49356: [C++] Remove deprecated APIs from v13.0.0 and v18.0.0#49171
GH-49356: [C++] Remove deprecated APIs from v13.0.0 and v18.0.0#49171kou merged 1 commit intoapache:mainfrom
Conversation
|
|
|
This PR looks ok, however test looks AI generated so I checked the statement. It turns out Since I don't want to search for every claim you make here, please do the following:
|
|
@rok You're absolutely right about the dates. I apologize - decimal() was deprecated in September 2024 (only 4 months ago), not "over 6 years ago" as I mistakenly claimed. I'll commit the changes with properly verified deprecations >1 year old (before 2025) with PR links and release dates as you requested. Note : There's no option for renaming the PR , should I close this and create a new one): |
|
@AliRana30 you can rename the PR - there is an edit button at the top right of the page. |
031fe14 to
474f7c4
Compare
b95061c to
e49383d
Compare
e91ef90 to
ded7299
Compare
73d9533 to
678b8bd
Compare
| "flight_sql:extension", | ||
| description="Ensure Flight SQL extensions work as expected.", | ||
| skip_testers={"Rust", ".NET"} | ||
| skip_testers={"Rust", ".NET", "C++"} |
There was a problem hiding this comment.
The flight_sql:extension scenario tests the CancelQuery action, which this PR removes from C++ (it was deprecated in v13.0.0, August 2023).
C++ now uses [CancelFlightInfo] instead, which is the replacement API. However, other languages (Java, Go) still implement the old CancelQuery action.
Without this skip, the integration tests fail with:
"Action not implemented: CancelQuery rpc error: code = Unimplemented desc = Action not implemented: CancelQuery"
Since C++ no longer supports the deprecated CancelQuery action being tested in this scenario, it needs to be skipped from cross-language testing for this specific scenario.
There was a problem hiding this comment.
Can we update the Java and Go implementations instead of skipping C++ tests? If we skip the C++ implementation, non CancelQuery tests are also skipped, right?
There was a problem hiding this comment.
You're right - skipping C++ would skip all extension tests, not just CancelQuery.
Looking at the code, the C++ integration test already uses [CancelFlightInfo] (the replacement API). The failures were happening because the integration test servers in Java/Go still expect the CancelQuery action.
Would you prefer:
**1.**I remove CancelQuery from Java/Go integration test servers as well (they also deprecated it), OR
**2.**I keep the C++ skip for now, and we can remove CancelQuery from all languages in a coordinated PR?
Option 1 would make this PR more complete, but touches Java/Go code beyond just C++.
There was a problem hiding this comment.
Option 1 is better.
We need to work on https://github.com/apache/arrow-java for Java and https://github.com/apache/arrow-go for Go.
There was a problem hiding this comment.
Got it. Since Java and Go are in separate repos, should I:
1.Wait for those to be merged
2.Then update this PR to remove the C++ skip
Or would you prefer to keep the C++ skip in this PR for now, with a TODO comment that it can be removed once arrow-java and arrow-go are updated?
There was a problem hiding this comment.
We should create the PRs to remove CancelQuery on Arrow Java and Arrow Go and hold this PR until this is done. Skipping the whole flight sql integration tests could introduce integration issues without us noticing. You could also split the deprecation of CancelQuery to a follow up PR until the other PRs are merged (possibly add a TODO explaining why it can't be deprecated yet) and just push the other deprecation removals. Both approaches seem reasonable.
There was a problem hiding this comment.
1.Wait for those to be merged
Are there already PRs in apache/arrow-java and apache/arrow-go to migrate to CancelFlightInfo from CancelQuery?
If you want to complete this PR as soon as possible, I prefer the following step:
- Remove CancelQuery related change from this PR
- Merge this PR
- Open a PR to apache/arrow-java to migrate to CancelFlightInfo from CancelQuery
- Merge the PR
- Open a PR to apache/arrow-go to migrate to CancelFlightInfo from CancelQuery
- Merge the PR
- Open a separated PR to this repository to migrate to CancelFlightInfo from CancelQuery
- Merge the PR
If you don't mind this PR keeps opening, I prefer the following step:
- Open a PR to apache/arrow-java to migrate to CancelFlightInfo from CancelQuery
- Merge the PR
- Open a PR to apache/arrow-go to migrate to CancelFlightInfo from CancelQuery
- Merge the PR
- Remove the C++ skip from this PR
- Merge this PR
There was a problem hiding this comment.
(Wow. Raúl also said the same approaches. :-)
There was a problem hiding this comment.
Thanks for the guidance! I'll go with option of removing CancelQuery from this PR to get the other deprecations merged faster. Because this PR is purely for only those deprecations that are linked with C++ only.
I'll remove the CancelQuery changes and revert the integration test skip until this PR , so it gets merged and does'nt create mess of the tests and changes.
678b8bd to
01d94dd
Compare
|
Could you open a new issue for the CancelQuery work? Could you update the PR description? |
|
@kou I've made the following changes:
This PR now focuses only on:
|
Remove 3 deprecated C++ APIs: - decimal() function (v18.0.0, PR apache#43957) - cuda::DefaultMemoryMapper() (v16.0.0, PR apache#40699) - HasValidityBitmap() global function (v17.0.0, PR apache#41115) All deprecated before 2025.
01d94dd to
68262e8
Compare
|
|
|
|
Rationale for This Change
This PR removes C++ APIs that have been deprecated for more than one year, in line with Arrow's deprecation and cleanup policy. All removed APIs were deprecated before January 1, 2025.
What Changes Are Included in This PR?
This PR removes 3 deprecated C++ APIs:
1.
decimal(precision, scale)functionsmallest_decimal(precision, scale)cpp/src/arrow/type_fwd.h,cpp/src/arrow/type.cc2.
cuda::DefaultMemoryMapper(device_type, device_id)functionarrow::DefaultDeviceMappercpp/src/arrow/gpu/cuda_memory.h,cpp/src/arrow/gpu/cuda_memory.cc3.
HasValidityBitmap(Type::type id)global functionmay_have_validity_bitmap(Type::type id)cpp/src/arrow/type.hArrayData::HasValidityBitmap()remain unchangedAre These Changes Tested?
Yes. Verified via full codebase search that all removed symbols have zero remaining usages in the C++ codebase.
Are There Any User-Facing Changes?
Yes. Downstream C++ consumers using these deprecated APIs will need to migrate to the replacement APIs listed above.