Repository navigation
Bump to Arrow 60 - #3257
Bump to Arrow 60#3257
Conversation
|
Nice PR, thanks @gabotechs looks like a smooth transition, are you planning to merge the update closer to the release? |
Whenever maintainers find more convenient, on my side no blockers to merge this as soon as possible. |
|
@comphead @kevinjqliu would it make sense to merge this now? Arrow/Parquet 60 has some pretty useful reader improvements around adaptive row-group pruning. I'm exploring live runtime pruning in the Iceberg reader, and the newer APIs make the implementation substantially cleaner and more effective. If the bump is otherwise ready, it would be really helpful to have this on main. |
…store 0.14 Point the DataFusion dependencies at a git revision of apache/datafusion main (4a5d58006e620bf6a175bb578be6652c3b330da9), which has moved to arrow/parquet 60.0.0 and object_store 0.14.2. Bump arrow, parquet, object_store and object_store_opendal (0.59.0, the first release on object_store 0.14) to match. iceberg-rust main is still on arrow 59, so pin iceberg and iceberg-storage-opendal to the head of apache/iceberg-rust#3257 (Arrow 60), which is not merged yet. Source changes for the new APIs: - Replace AlignedArrowStreamReader with arrow's ArrowArrayStreamReader. Arrow 60 makes the FFI_ArrowArrayStream callbacks private, and since arrow 59 from_ffi_and_data_type realigns under-aligned buffers itself, which is the reason the custom reader existed. The Decimal128 realignment test moves to scan.rs. - Parquet 60 moves the page index behind ParquetMetaData::page_index(). - object_store 0.14 adds GetResult::extensions. - OffsetBuffer::first/last return values, schema metadata is arrow's Metadata type, fb_to_schema is deprecated for try_fb_to_schema, and FFI_ArrowSchema::with_metadata is unsafe. - HashJoinExec::with_dynamic_filter_expr is deprecated with no replacement; allow it in DynamicFilterJoinExec. - Arrow 60 accepts empty entries in unsorted Variant dictionaries (apache/arrow-rs#10352), so two metadata encodings the empty-key workaround used to reject now pass through unchanged, as they do in Spark.
Bumps arrow and parquet to 60.0 and pins DataFusion to bohmian-ai/datafusion 202c5e101c8e (v55.1.0 rebased on apache main with arrow 60). Ports the parquet 60 page-index API from apache#3257, takes TableProvider::scan projections as slices, and keeps the pre-60 Arrow map child names explicit in the name-mapping fixture. Claude-Session: https://claude.ai/code/session_01KGc5N2RkiRMFtCmHBpXqRV
we would prob need to consider DF 55 migration as well. |
This would be for apache/datafusion-iceberg only, right? I think we could move forward in this repository, with a follow-up Arrow+DF in that repo? |
Oh yes, thats a good point, which generates some complexity with dependencies. We can move with Arrow 60, and datafusion-iceberg is already with DF 55 |
…store 0.14 Point the DataFusion dependencies at a git revision of apache/datafusion main (350ca76505c9513d16e7dd9910bcd9bde2c911ed), which is on arrow/parquet 60.0.0 and object_store 0.14.2. Bump arrow, parquet, object_store and object_store_opendal (0.59.0, the first release on object_store 0.14) to match. iceberg-rust main is still on arrow 59, and the head of apache/iceberg-rust#3257 (Arrow 60) is 42 commits behind the main revision Comet builds against. Pin iceberg and iceberg-storage-opendal to a branch on the andygrove/iceberg-rust fork that merges apache/iceberg-rust main (1f3bc34) into that PR. Source changes for the new APIs: - Arrow 60 makes the FFI_ArrowArrayStream callbacks private, so the decimal-realignment test builds the import with from_ffi_and_data_type. - Parquet 60 moves the page index behind ParquetMetaData::page_index(), a PageIndexProvider with per-row-group access. - DataFusion's coerce_int96 and coerce_int96_tz are typed (DFTimeUnit, ConfigTimeZone) instead of strings. - OffsetBuffer::first/last return values and schema metadata is arrow's Metadata type. - HashJoinExec::with_dynamic_filter_expr, SortExec::with_dynamic_filter_expr, ExecutionPlan::with_preserve_order and ParquetAccessPlan::into_overall_row_selection are deprecated; allow them where Comet still needs them. - Arrow 60's MapBuilder names map children key/value by default, so make_map_c names them keys/values explicitly to keep exercising non-canonical names. - datafusion-spark's SparkXxhash64 now ignores the hidden children of a NULL struct and keeps the running hash for nested dictionaries (apache/datafusion #25774, #25813), so the tests that pinned those differences assert equality.
Move the andygrove/iceberg-rust fork pin to apache/iceberg-rust#3257 with apache/iceberg-rust main at 46e51d8 merged in. Main now includes apache/iceberg-rust#3539, which skips NaN min and max statistics when aggregating Parquet column bounds, so the native writer no longer records NaN manifest bounds for all-NaN float and double columns on Parquet 60. This fixes the Iceberg TestAggregatePushDown testNaN and testAllNaN failures, re-enables the NaN manifest metrics test, and drops the known issue from the docs. Closes #7242.
|
DataFusion is preparing for the 56 release, which depends on Arrow 60. As cc @dannycjones in case you have time for reviewing this. |
0d5eb12 to
73e772c
Compare
|
curious what is our strategy for iceberg crate version to datafusion version mapping? i see 0.11.0 is using df 53 (477a1e5) and this one is upgrading to arrow 60. what should 0.12.0 release contain? |
|
We'd like to see this land as well. We use the |
|
@kevinjqliu I see 0.11 was released with:
It'd be nice to have 0.12 contain:
Unfortunately, having this project depend on Arrow 58 will render it unusable to most people already depending on the Arrow ecosystem, as major versions will conflict. |
|
Could you also bump |
|
If its easier, I'm happy to do that myself in a follow up. |
|
Happy to do it in this PR yes, but would like confirmation from maintainers here in case they prefer to keep updates separate in different PRs. |
|
FWIW - that will actually save a dependency here, as some underlying crates (like |
|
I think we should keep the |
|
No problem, I'll have a PR soon. |
We shouldn't really have to worry about DataFusion too much going forward. Ideally I think this crate should try to jump to new major arrow-rs versions pretty quickly. I think 0.12.0 should have Arrow-rs 60.x. 61.0.0 will be out next month, but I think we should shoot for an upgrade and release before that. |
|
makes sense, i merged the reqwest 0.13 upgrade pr (#3666) and rebased this pr on latest main to include that change |
oh right, 🤦 i forgot we removed the datafusion dependency (i dont see it in https://github.com/apache/iceberg-rust/blob/b879b9626754501f025516ce0f0e9535fa6fd414/Cargo.toml anymore) And this PR is for Arrow. Might be a good idea to do a 0.12 release soon since we had lots of changes since the 0.11 release @mbutrovich @gabotechs think this is good to merge? |
mbutrovich
left a comment
There was a problem hiding this comment.
Approved pending CI.
|
adding to merge queue 😄 |
|
thanks @gabotechs and everyone for the review ❤️ |
Update to the current apache/iceberg-rust#3257 head after its rebase and adapt to the latest partition spec API.
Builds on apache#7249, which tracks DataFusion branch-56 (56.0.0-rc1), with current main merged in and the DataFusion dependencies moved to DataFusion's main branch. - datafusion crates on apache/datafusion's main branch (57.0.0-dev), locked at 6c42917 - datafusion-spark on apache/datafusion-spark's main branch, locked at 4cd969d, because apache/datafusion#27337 moved the crate out of DataFusion. That repository builds against DataFusion branch-56, so a [patch] section moves its DataFusion dependencies to DataFusion main - arrow/parquet 60.1.0, object_store 0.14.2 - iceberg and iceberg-storage-opendal on apache/iceberg-rust@6c97391, the apache/iceberg-rust#3257 (Arrow 60) merge, instead of the andygrove/iceberg-rust fork. It contains the revision main uses (de0a6ac), and main's opendal-hdfs-native feature is kept - native/native-executors stays as on main, and its CI job stays off as in apache#7249 - source changes for the Arrow 60, Parquet 60 and DataFusion API changes as in apache#7249 Co-authored-by: Andy Grove <[email protected]>
Builds on apache#7249, which tracks DataFusion branch-56 (56.0.0-rc1), with current main merged in and the DataFusion dependencies moved to DataFusion's main branch. - datafusion crates on apache/datafusion's main branch (57.0.0-dev), locked at 73e0b03 - datafusion-spark on apache/datafusion-spark's main branch, locked at 21334bb, because apache/datafusion#27337 moved the crate out of DataFusion. That repository builds against DataFusion branch-56, so a [patch] section moves its DataFusion dependencies to DataFusion main - arrow/parquet 60.1.0, object_store 0.14.2 - iceberg and iceberg-storage-opendal on apache/iceberg-rust@6c97391, the apache/iceberg-rust#3257 (Arrow 60) merge, instead of the andygrove/iceberg-rust fork. It contains the revision main uses (de0a6ac), and main's opendal-hdfs-native feature is kept - native/native-executors stays as on main, and its CI job stays off as in apache#7249 - source changes for the Arrow 60, Parquet 60 and DataFusion API changes as in apache#7249 Co-authored-by: Andy Grove <[email protected]>
Which issue does this PR close?
What changes are included in this PR?
Bumps the
arrowdependency to 60, addressing all breaking API changes.Are these changes tested?
Yes, by existing tests
AI Disclosure
The code is fully generated by Codex and reviewed by me