GH-49252: [GLib] Deprecate Feather features#49673
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR deprecates Apache Arrow GLib’s Feather APIs (notably Feather V1 reader/writer surfaces) and standardizes deprecation annotations by switching to GARROW_DEPRECATED* macros.
Changes:
- Add
GARROW_DEPRECATED_IN_24_0/GARROW_DEPRECATED_IN_24_0_FOR(...)annotations to Feather-related APIs. - Replace direct GLib deprecation attributes (
G_GNUC_DEPRECATED*) with Arrow-GLib wrappers (GARROW_DEPRECATED*). - Deprecate raw
GIO*Streamaccessors viaGARROW_DEPRECATED.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| c_glib/arrow-glib/table.h | Deprecates Feather write properties + table Feather writer API. |
| c_glib/arrow-glib/reader.h | Switches deprecated annotations and deprecates Feather file reader APIs. |
| c_glib/arrow-glib/output-stream.h | Uses GARROW_DEPRECATED for deprecated raw GIO output stream accessor. |
| c_glib/arrow-glib/input-stream.h | Uses GARROW_DEPRECATED for deprecated raw GIO input stream accessor. |
Comments suppressed due to low confidence (1)
c_glib/arrow-glib/table.h:176
GARROW_DEPRECATED_IN_24_0_FOR(garrow_record_batch_file_writer_new)suggests a direct replacement, butgarrow_table_write_as_feather()writes a whole table in Feather format whereasgarrow_record_batch_file_writer_new()only constructs a writer and requires additional steps (schema, write_table, close) and produces IPC file format rather than Feather. Consider dropping_FOR(useGARROW_DEPRECATED_IN_24_0) or pointing at the most appropriate entry point for the migration (for example the writer creation +garrow_record_batch_writer_write_table/garrow_record_batch_writer_close).
GARROW_AVAILABLE_IN_0_17
GARROW_DEPRECATED_IN_24_0_FOR(garrow_record_batch_file_writer_new)
gboolean
garrow_table_write_as_feather(GArrowTable *table,
GArrowOutputStream *sink,
GArrowFeatherWriteProperties *properties,
GError **error);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| GARROW_AVAILABLE_IN_0_17 | ||
| GARROW_DEPRECATED_IN_24_0_FOR(garrow_write_options_new) |
There was a problem hiding this comment.
GARROW_DEPRECATED_IN_24_0_FOR(garrow_write_options_new) is likely misleading here: GArrowFeatherWriteProperties configures Feather write behavior, while GArrowWriteOptions is for Arrow IPC serialization. If there isn’t a 1:1 replacement API, prefer using GARROW_DEPRECATED_IN_24_0 (without _FOR) or point to a closer replacement specific to the intended migration path (e.g., IPC file writer + options).
| GARROW_DEPRECATED_IN_24_0_FOR(garrow_write_options_new) | |
| GARROW_DEPRECATED_IN_24_0 |
| garrow_feather_file_reader_get_version(GArrowFeatherFileReader *reader); | ||
|
|
||
| GARROW_AVAILABLE_IN_ALL | ||
| GARROW_DEPRECATED_IN_24_0_FOR(garrow_record_batch_reader_read_all) |
There was a problem hiding this comment.
GARROW_DEPRECATED_IN_24_0_FOR(garrow_record_batch_reader_read_all) doesn’t look like a valid replacement: garrow_record_batch_reader_read_all() takes a GArrowRecordBatchReader*, but this API works on GArrowFeatherFileReader*, and GArrowRecordBatchFileReader is not a GArrowRecordBatchReader. If there’s no single drop-in replacement, use GARROW_DEPRECATED_IN_24_0 (no _FOR) or reference a concrete, correct alternative flow (e.g., garrow_record_batch_file_reader_new() + reading record batches).
| GARROW_DEPRECATED_IN_24_0_FOR(garrow_record_batch_reader_read_all) | |
| GARROW_DEPRECATED_IN_24_0 |
|
@raulcd Can we merge this for 24.0.0? This just adds deprecation warnings to Feature related features. |
|
No! We can merge this as-is for 24.0.0! |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 1f94910. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
We'll deprecate Feather V1 reader/writer. See also the discussion at https://lists.apache.org/thread/1npvnhjb1xwz09zh8vnd079zt2q4o08l .
We can use IPC file reader/writer for Feather V2.
What changes are included in this PR?
GARROW_DEPRECATED()/GARROW_DEPRECATED_FOR()instead ofG_GNUC_DEPRECATED()/G_GNUC_DEPRECATED_FOR().Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.