WIP: Update to arrow/parquet 59.2.0 - #24030
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
| if let Some(hint) = metadata_size_hint { | ||
| inner = inner.with_footer_size_hint(hint) | ||
| }; | ||
| let location = partitioned_file.object_meta.location; |
There was a problem hiding this comment.
ParquetObjectReader is deprecated due to
| filename: String, | ||
| metadata: Arc<ParquetMetaData>, | ||
| inner: ParquetObjectReader, | ||
| object_store: Arc<dyn ObjectStore>, |
There was a problem hiding this comment.
this is arguable a nicer example now, as it avoids having to wrap the ParquetObjectReader
| let to_write = RecordBatch::try_from_iter(data).unwrap(); | ||
| let path = object_store::path::Path::from(format!("{file_num}.parquet").as_str()); | ||
| let object_store_writer = ParquetObjectWriter::new(Arc::clone(&store) as _, path); | ||
| let object_store_writer = BufWriter::new(Arc::clone(&store) as _, path); |
There was a problem hiding this comment.
ParquetObjectWriter wsas deprecated: apache/arrow-rs#10308
ParquetObjectWriter is a thin wrapper around BufWriter: https://docs.rs/parquet/latest/src/parquet/arrow/async_writer/store.rs.html#72-74
| ) -> BoxFuture<'_, datafusion::parquet::errors::Result<Bytes>> { | ||
| println!("get_bytes: {} Reading range {:?}", self.filename, range); | ||
| self.inner.get_bytes(range) | ||
| let object_store = Arc::clone(&self.object_store); |
There was a problem hiding this comment.
This is due to inlining the logic for ParquetObjectReader
It turns out that DataFusion already has its own copy of the logic of the ParquetObjectReader between ParquetFileReader and CachedParquetFileReader. I have a PR that will unify them:
| options: Option<&'a ArrowReaderOptions>, | ||
| ) -> BoxFuture<'a, parquet::errors::Result<Arc<ParquetMetaData>>> { | ||
| self.inner.get_metadata(options) | ||
| let object_meta = self.partitioned_file.object_meta.clone(); |
There was a problem hiding this comment.
THe need for this replication is removed by unifying the structures, as I propose to do do in
|
|
||
| **Migration guide:** | ||
|
|
||
| If your [`AsyncFileReader`] implementation exists mainly to read from an |
There was a problem hiding this comment.
This will need to get updated if we merge this one first
| acc1 = merge(acc1, acc2)?; | ||
|
|
||
| assert_eq!(acc1.size(), 282); | ||
| assert_eq!(acc1.size(), 290); |
There was a problem hiding this comment.
… arrow spec (#10297)" (#10506) # Which issue does this PR close? N/A cc @Jefffrey and @rluvaton # Rationale for this change #10297 changed the default map field names from `keys`/`values` to `key`/`value` to match the Arrow spec. This is a good change to align with the spec, but it is a breaking change: it broke the DataFusion upgrade (apache/datafusion#24030) because data produced elsewhere (e.g. by other Arrow implementations) uses the old `keys`/`values` field names, causing schema mismatches such as: ``` InvalidArgumentError("Incorrect datatype for StructArray field \"metadata\", expected Map(\"entries\": non-null Struct(\"keys\": non-null Utf8, \"values\": Utf8), unsorted) got Map(\"entries\": non-null Struct(\"key\": non-null Utf8, \"value\": Utf8), unsorted)") ``` See discussion on #10297: #10297 (comment) We should hold the field name change for the next breaking release to minimize downstream churn on a minor release, rather than ship it in a minor release. # What changes are included in this PR? - Reverts b963ecf (#10297), restoring the default map field names to `keys`/`values` (plural). - Updates a test added by #10475 after #10297 merged # Are these changes tested? Existing tests. # Are there any user-facing changes? Yes: this reverts the default `MapFieldNames` back to `keys`/`values` (as it was prior to #10297), rather than `key`/`value`. The intent is to reapply #10297 as part of the next breaking release.
…rrow-rs 59.2.0 Bumping arrow-rs changed the underlying buffer memory footprint reported by ArrayData::get_slice_memory_size() for this scenario, shifting the accumulator's accounted size from 282 to 290 bytes. This is an expected size increase from the dependency bump, not an accounting regression: the merge behavior and both sibling tests (does_not_over_account_memory_distinct/_ordered) are unaffected.
e0fe024 to
f19d7ce
Compare
arrow-rs 59.2.0 added validation that rejects constructing a MapArray whose key field is nullable (apache/arrow-rs#10272), which is required by the Arrow spec. The Map test cases in round_trip_scalar_values_and_data_types built their key field with nullable=true, so decoding the IPC-encoded ScalarValue now fails with "Map key field must not be nullable".
| "entries", | ||
| DataType::Struct(Fields::from(vec![ | ||
| Field::new("key", DataType::Int32, true), | ||
| Field::new("key", DataType::Int32, false), |
There was a problem hiding this comment.
this is necessary due to MapArray becoming more strict in
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24030 +/- ##
==========================================
- Coverage 80.86% 80.86% -0.01%
==========================================
Files 1101 1101
Lines 375446 375464 +18
Branches 375446 375464 +18
==========================================
+ Hits 303596 303606 +10
- Misses 53757 53759 +2
- Partials 18093 18099 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Which issue does this PR close?
59.2.0(July 2026) arrow-rs#9879Rationale for this change
Upgrade to latest arrow / parquet library
What changes are included in this PR?
Are these changes tested?
yes, by CI
Are there any user-facing changes?
Not yet