You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kacper-li-airspace-intelligence (via GitHub)" <gi...@apache.org> on 2026/09/15 07:38:15 UTC
[PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
kacper-li-airspace-intelligence opened a new pull request, #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224
## What changes are included in this PR?
`ManifestList::parse_with_version` collects the entire Avro reader into a `Value::Array` before deserializing it. Each manifest stays in memory as a `Value::Record`, with owned field names, strings and nested values. `from_value` then borrows that array while allocating the typed records, so both representations coexist.
This becomes expensive when a snapshot contains many manifests. For example, `FastAppendAction::existing_manifest` loads the current manifest list on each append, even if the new append is small. The temporary allocation cost grows with the existing list, not the size of the append.
Convert each Avro record into its final `ManifestFile` as it is read, then drop the generic value before reading the next record. V1 keeps its reader-schema projection; V2/V3 keep the writer schema, field aliases and defaults. The input bytes and final typed list still occupy memory. Parsing still returns an error without a partial list, although a conversion error can now precede a later Avro decoding error.
Remove the unused whole-list conversion methods and keep the serialization wrappers used by the existing tests behind `cfg(test)`.
## Are these changes tested?
The added `manifest_list_memory` test generates 10,000 distinct manifest paths with one partition summary per entry. It checks every parsed entry and measures peak live allocations with a test-only wrapper around `System`. On aarch64 macOS, Rust 1.96.0, release mode:
| Uncompressed V2 list: 692,289 input bytes | Peak additional live allocations |
| --- | ---: |
| Main at `9db0b685` | 32,874,672 bytes |
| This change | 6,040,920 bytes |
The 16 MiB regression budget fails on main and passes with the patch. Input generation is excluded from the measurement. Snappy and Zstandard cases also pass; the test additionally checks V1/V2/V3 round trips, empty lists, V1 defaults when read as V2, and rejection of truncated input.
```sh
cargo +stable test -p iceberg --release --test manifest_list_memory --locked -- --nocapture
```
To reproduce the baseline, keep the added test and restore `crates/iceberg/src/spec/manifest_list/{mod.rs,_serde.rs}` from `9db0b685`, then run the same command.
All 1,691 core unit tests pass, including the existing schema-projection, field-alias and encrypted manifest-list tests. Clippy and formatting pass with the repository's pinned nightly toolchain.
## AI Disclosure
AI assisted with the implementation, test and description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "kacper-li-airspace-intelligence (via GitHub)" <gi...@apache.org>.
kacper-li-airspace-intelligence commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4267159180
##########
crates/iceberg/tests/manifest_list_memory.rs:
##########
@@ -0,0 +1,200 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Manifest-list compatibility and peak live-allocation regression.
+
+use std::alloc::{GlobalAlloc, Layout, System};
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering::Relaxed;
+use std::time::Instant;
+
+use apache_avro::types::Value;
+use apache_avro::{Codec, Reader, Writer};
+use iceberg::io::FileIO;
+use iceberg::spec::{
+ FieldSummary, FormatVersion, ManifestContentType, ManifestFile, ManifestList,
+ ManifestListWriter,
+};
+
+struct CountingAllocator;
+static LIVE: AtomicUsize = AtomicUsize::new(0);
+static PEAK: AtomicUsize = AtomicUsize::new(0);
+
+fn allocated(bytes: usize) {
+ PEAK.fetch_max(LIVE.fetch_add(bytes, Relaxed) + bytes, Relaxed);
+}
+
+// The allocator is confined to this test executable. Every allocation is tracked,
+// including those made before measurement, so deallocation cannot underflow LIVE.
+unsafe impl GlobalAlloc for CountingAllocator {
+ unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc_zeroed(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+ unsafe { System.dealloc(ptr, layout) };
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ }
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, size: usize) -> *mut u8 {
+ let new_ptr = unsafe { System.realloc(ptr, layout, size) };
+ if !new_ptr.is_null() {
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ allocated(size);
+ }
+ new_ptr
+ }
+}
+
+#[global_allocator]
+static ALLOCATOR: CountingAllocator = CountingAllocator;
Review Comment:
Thanks for pointing it out I missed this, I folded the tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "kacper-li-airspace-intelligence (via GitHub)" <gi...@apache.org>.
kacper-li-airspace-intelligence commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4259670059
##########
crates/iceberg/src/spec/manifest_list/mod.rs:
##########
@@ -58,22 +57,39 @@ pub struct ManifestList {
impl ManifestList {
/// Parse manifest list from bytes.
+ /// Convert each Avro value immediately, retaining only the final typed entries.
+ /// A decoding or conversion error still discards the entire result.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) -> Result<ManifestList> {
match version {
FormatVersion::V1 => {
let reader = Reader::with_schema(&MANIFEST_LIST_AVRO_SCHEMA_V1, bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV1>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV1>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
FormatVersion::V2 => {
let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV2>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV2>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
FormatVersion::V3 => {
let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV3>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV3>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
}
}
Review Comment:
Done. I merged `main` and use your `into_deser_iter` version. Peak is now 4,355,177 bytes, against 31,194,672 on `main`.
##########
crates/iceberg/tests/manifest_list_memory.rs:
##########
@@ -0,0 +1,200 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Manifest-list compatibility and peak live-allocation regression.
+
+use std::alloc::{GlobalAlloc, Layout, System};
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering::Relaxed;
+use std::time::Instant;
+
+use apache_avro::types::Value;
+use apache_avro::{Codec, Reader, Writer};
+use iceberg::io::FileIO;
+use iceberg::spec::{
+ FieldSummary, FormatVersion, ManifestContentType, ManifestFile, ManifestList,
+ ManifestListWriter,
+};
+
+struct CountingAllocator;
+static LIVE: AtomicUsize = AtomicUsize::new(0);
+static PEAK: AtomicUsize = AtomicUsize::new(0);
+
+fn allocated(bytes: usize) {
+ PEAK.fetch_max(LIVE.fetch_add(bytes, Relaxed) + bytes, Relaxed);
+}
+
+// The allocator is confined to this test executable. Every allocation is tracked,
+// including those made before measurement, so deallocation cannot underflow LIVE.
+unsafe impl GlobalAlloc for CountingAllocator {
+ unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc_zeroed(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+ unsafe { System.dealloc(ptr, layout) };
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ }
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, size: usize) -> *mut u8 {
+ let new_ptr = unsafe { System.realloc(ptr, layout, size) };
+ if !new_ptr.is_null() {
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ allocated(size);
+ }
+ new_ptr
+ }
+}
+
+#[global_allocator]
+static ALLOCATOR: CountingAllocator = CountingAllocator;
Review Comment:
I moved the round trip, empty, truncated and V1-as-V2 cases into separate unit tests in `spec/manifest_list/mod.rs`. I removed the integration test. The before/after numbers are now in the description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4235390475
##########
crates/iceberg/src/spec/manifest_list/mod.rs:
##########
@@ -58,22 +57,39 @@ pub struct ManifestList {
impl ManifestList {
/// Parse manifest list from bytes.
+ /// Convert each Avro value immediately, retaining only the final typed entries.
+ /// A decoding or conversion error still discards the entire result.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) -> Result<ManifestList> {
match version {
FormatVersion::V1 => {
let reader = Reader::with_schema(&MANIFEST_LIST_AVRO_SCHEMA_V1, bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV1>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV1>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
FormatVersion::V2 => {
let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV2>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV2>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
FormatVersion::V3 => {
let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV3>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV3>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
}
}
Review Comment:
#3354 merged today. It upgraded to `apache-avro` 0.22 and reads manifest entries with `Reader::into_deser_iter`, which decodes straight into the serde types without building an `apache_avro::types::Value`. The [`Resolved`](https://github.com/apache/iceberg-rust/blob/e8ec7f8403554496db976700eb993459e0ab7f9b/crates/iceberg/src/avro/deserializer.rs#L56-L64) adapter it added handles writer record names, unions, and numeric widening against the writer schema.
What do you think about rebasing and reading manifest lists the same way as the [manifest entry reader](https://github.com/apache/iceberg-rust/blob/e8ec7f8403554496db976700eb993459e0ab7f9b/crates/iceberg/src/spec/manifest/mod.rs#L114-L136)? Something like:
```rust
/// Parse manifest list from bytes.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) -> Result<ManifestList> {
let reader = Reader::new(bs)?;
let entries = match version {
FormatVersion::V1 => reader
.into_deser_iter::<Resolved<_serde::ManifestFileV1>>()
.map(|entry| entry?.0.try_into())
.collect::<Result<Vec<_>>>()?,
FormatVersion::V2 => reader
.into_deser_iter::<Resolved<_serde::ManifestFileV2>>()
.map(|entry| entry?.0.try_into())
.collect::<Result<Vec<_>>>()?,
FormatVersion::V3 => reader
.into_deser_iter::<Resolved<_serde::ManifestFileV3>>()
.map(|entry| entry?.0.try_into())
.collect::<Result<Vec<_>>>()?,
};
Ok(ManifestList { entries })
}
```
This removes the per-record `Value` as well as the whole-list one. With it, your `manifest_list_memory` test reports a peak of 4,355,177 bytes for the uncompressed list on my machine, against 6,040,920 at the head commit. The field aliases and V1 defaults on `ManifestFileV2` and `ManifestFileV3` are serde attributes, so they carry over. V1 would read against the writer schema instead of `MANIFEST_LIST_AVRO_SCHEMA_V1`, as the manifest entry reader already does for V1 manifests. The two doc lines about converting each Avro value would go too.
This is also the "Read manifest lists without `Value`" task in #3262.
##########
crates/iceberg/tests/manifest_list_memory.rs:
##########
@@ -0,0 +1,200 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Manifest-list compatibility and peak live-allocation regression.
+
+use std::alloc::{GlobalAlloc, Layout, System};
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering::Relaxed;
+use std::time::Instant;
+
+use apache_avro::types::Value;
+use apache_avro::{Codec, Reader, Writer};
+use iceberg::io::FileIO;
+use iceberg::spec::{
+ FieldSummary, FormatVersion, ManifestContentType, ManifestFile, ManifestList,
+ ManifestListWriter,
+};
+
+struct CountingAllocator;
+static LIVE: AtomicUsize = AtomicUsize::new(0);
+static PEAK: AtomicUsize = AtomicUsize::new(0);
+
+fn allocated(bytes: usize) {
+ PEAK.fetch_max(LIVE.fetch_add(bytes, Relaxed) + bytes, Relaxed);
+}
+
+// The allocator is confined to this test executable. Every allocation is tracked,
+// including those made before measurement, so deallocation cannot underflow LIVE.
+unsafe impl GlobalAlloc for CountingAllocator {
+ unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc_zeroed(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+ unsafe { System.dealloc(ptr, layout) };
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ }
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, size: usize) -> *mut u8 {
+ let new_ptr = unsafe { System.realloc(ptr, layout, size) };
+ if !new_ptr.is_null() {
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ allocated(size);
+ }
+ new_ptr
+ }
+}
+
+#[global_allocator]
+static ALLOCATOR: CountingAllocator = CountingAllocator;
Review Comment:
This would be the first integration test in `crates/iceberg`. It brings a new test binary and an `unsafe` global allocator, and it folds the format checks into one test so they can share the allocator. Could the round trips, the empty lists, the truncated input, and the V1 file read as V2 move into the unit tests in `spec/manifest_list/mod.rs`, next to `test_parse_manifest_list_v1`? Those cases aren't covered there today, and as separate tests a failure in one wouldn't hide the others.
For the allocation budget, what do you think about leaving it out of the suite and keeping the before and after numbers in the PR description? The test also times the parse with `Instant` and prints it without checking it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4259822125
##########
crates/iceberg/tests/manifest_list_memory.rs:
##########
@@ -0,0 +1,200 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Manifest-list compatibility and peak live-allocation regression.
+
+use std::alloc::{GlobalAlloc, Layout, System};
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering::Relaxed;
+use std::time::Instant;
+
+use apache_avro::types::Value;
+use apache_avro::{Codec, Reader, Writer};
+use iceberg::io::FileIO;
+use iceberg::spec::{
+ FieldSummary, FormatVersion, ManifestContentType, ManifestFile, ManifestList,
+ ManifestListWriter,
+};
+
+struct CountingAllocator;
+static LIVE: AtomicUsize = AtomicUsize::new(0);
+static PEAK: AtomicUsize = AtomicUsize::new(0);
+
+fn allocated(bytes: usize) {
+ PEAK.fetch_max(LIVE.fetch_add(bytes, Relaxed) + bytes, Relaxed);
+}
+
+// The allocator is confined to this test executable. Every allocation is tracked,
+// including those made before measurement, so deallocation cannot underflow LIVE.
+unsafe impl GlobalAlloc for CountingAllocator {
+ unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc_zeroed(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+ unsafe { System.dealloc(ptr, layout) };
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ }
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, size: usize) -> *mut u8 {
+ let new_ptr = unsafe { System.realloc(ptr, layout, size) };
+ if !new_ptr.is_null() {
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ allocated(size);
+ }
+ new_ptr
+ }
+}
+
+#[global_allocator]
+static ALLOCATOR: CountingAllocator = CountingAllocator;
Review Comment:
> Those cases aren't covered there today, and as separate tests a failure in one wouldn't hide the others.
I got part of this wrong, sorry for the churn. `test_parse_manifest_list_v1`, `test_parse_manifest_list_v2` and `test_parse_manifest_list_v3` in the same module already write a list with `ManifestListWriter` and parse it back. [`test_manifest_list_writer_v1_as_v2`](https://github.com/apache/iceberg-rust/blob/3db218ec660658ca33acfebcf849006d727065c9/crates/iceberg/src/spec/manifest_list/writer.rs#L460-L503) in `writer.rs` writes a V1 list and parses it as V2, which is what `test_parse_manifest_list_v1_as_v2` checks. Only the empty and truncated cases are new.
What do you think about dropping `test_parse_manifest_list_round_trip` and `test_parse_manifest_list_v1_as_v2`? The round trip does add inputs the existing tests don't have: a non-empty `key_metadata`, and a null `contains_nan` and `upper_bound`, which go through the union handling in `Resolved`. Setting those on an entry in the existing `test_parse_manifest_list_v*` tests would keep that coverage without a second set of round trips.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "kacper-li-airspace-intelligence (via GitHub)" <gi...@apache.org>.
kacper-li-airspace-intelligence commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4267159180
##########
crates/iceberg/tests/manifest_list_memory.rs:
##########
@@ -0,0 +1,200 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Manifest-list compatibility and peak live-allocation regression.
+
+use std::alloc::{GlobalAlloc, Layout, System};
+use std::sync::atomic::AtomicUsize;
+use std::sync::atomic::Ordering::Relaxed;
+use std::time::Instant;
+
+use apache_avro::types::Value;
+use apache_avro::{Codec, Reader, Writer};
+use iceberg::io::FileIO;
+use iceberg::spec::{
+ FieldSummary, FormatVersion, ManifestContentType, ManifestFile, ManifestList,
+ ManifestListWriter,
+};
+
+struct CountingAllocator;
+static LIVE: AtomicUsize = AtomicUsize::new(0);
+static PEAK: AtomicUsize = AtomicUsize::new(0);
+
+fn allocated(bytes: usize) {
+ PEAK.fetch_max(LIVE.fetch_add(bytes, Relaxed) + bytes, Relaxed);
+}
+
+// The allocator is confined to this test executable. Every allocation is tracked,
+// including those made before measurement, so deallocation cannot underflow LIVE.
+unsafe impl GlobalAlloc for CountingAllocator {
+ unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 {
+ let ptr = unsafe { System.alloc_zeroed(layout) };
+ if !ptr.is_null() {
+ allocated(layout.size());
+ }
+ ptr
+ }
+
+ unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
+ unsafe { System.dealloc(ptr, layout) };
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ }
+
+ unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, size: usize) -> *mut u8 {
+ let new_ptr = unsafe { System.realloc(ptr, layout, size) };
+ if !new_ptr.is_null() {
+ LIVE.fetch_sub(layout.size(), Relaxed);
+ allocated(size);
+ }
+ new_ptr
+ }
+}
+
+#[global_allocator]
+static ALLOCATOR: CountingAllocator = CountingAllocator;
Review Comment:
Thanks for pointing it out I missed this. I folded the tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "kacper-li-airspace-intelligence (via GitHub)" <gi...@apache.org>.
kacper-li-airspace-intelligence commented on PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#issuecomment-6185721537
@mbutrovich thank you for all your comments and the review. I linked the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4259833708
##########
crates/iceberg/src/spec/manifest_list/mod.rs:
##########
@@ -59,25 +58,22 @@ pub struct ManifestList {
impl ManifestList {
/// Parse manifest list from bytes.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) -> Result<ManifestList> {
- match version {
- FormatVersion::V1 => {
- let reader = Reader::builder(bs)
- .reader_schema(&MANIFEST_LIST_AVRO_SCHEMA_V1)
- .build()?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV1>(&values)?.try_into()
- }
- FormatVersion::V2 => {
- let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV2>(&values)?.try_into()
- }
- FormatVersion::V3 => {
- let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV3>(&values)?.try_into()
- }
- }
+ let reader = Reader::new(bs)?;
+ let entries = match version {
+ FormatVersion::V1 => reader
+ .into_deser_iter::<Resolved<_serde::ManifestFileV1>>()
+ .map(|entry| entry?.0.try_into())
+ .collect::<Result<Vec<_>>>()?,
Review Comment:
Java writes V1 manifest lists with the field names `added_files_count`, `existing_files_count` and `deleted_files_count` (see [`V1Metadata.MANIFEST_LIST_SCHEMA`](https://github.com/apache/iceberg/blob/db43b88a6c0d470bd4f8acf08a18328750aeed20/core/src/main/java/org/apache/iceberg/V1Metadata.java#L32-L45) and [`ManifestFile`](https://github.com/apache/iceberg/blob/db43b88a6c0d470bd4f8acf08a18328750aeed20/api/src/main/java/org/apache/iceberg/ManifestFile.java#L56-L61)), which are the names in the [spec's manifest list fields](https://github.com/apache/iceberg/blob/db43b88a6c0d470bd4f8acf08a18328750aeed20/format/spec.md?plain=1#L1023-L1025). [`ManifestFileV1`](https://github.com/apache/iceberg-rust/blob/3db218ec660658ca33acfebcf849006d727065c9/crates/iceberg/src/spec/manifest_list/_serde.rs#L97-L110) only knows the older `added_data_files_count` names and has no aliases. So for a V1 list written by current Java, those three counts parse as `None`.
I checked this by writing a one-entry list with Java's V1 Avro schema and parsing it as V1. `added_files_count` and `existing_files_count` come back `None`, while `added_rows_count` (same name in both) comes back `Some(9)`. `main` at `b154603a` returns the same thing, so this isn't from this PR. Before, resolution against `MANIFEST_LIST_AVRO_SCHEMA_V1` filled `added_data_files_count` with its null default, so serde never saw Java's names. Now that V1 reads against the writer schema, the `#[serde(alias = ...)]` approach `ManifestFileV2` already uses works for V1 too. Adding the three aliases to `ManifestFileV1` returns `Some(3)` and `Some(0)` for that list.
The spec treats a null count as non-zero, so planning stays correct, but the counts are lost. As I read [`existing_manifest`](https://github.com/apache/iceberg-rust/blob/3db218ec660658ca33acfebcf849006d727065c9/crates/iceberg/src/transaction/append.rs#L126-L151), a fast append on such a table copies these entries into the new manifest list, so the nulls get written back. Since this is a separate bug, could you open an issue for it? The fix plus a test with Java's V1 names would make a small follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "kacper-li-airspace-intelligence (via GitHub)" <gi...@apache.org>.
kacper-li-airspace-intelligence commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4267244254
##########
crates/iceberg/src/spec/manifest_list/mod.rs:
##########
@@ -59,25 +58,22 @@ pub struct ManifestList {
impl ManifestList {
/// Parse manifest list from bytes.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) -> Result<ManifestList> {
- match version {
- FormatVersion::V1 => {
- let reader = Reader::builder(bs)
- .reader_schema(&MANIFEST_LIST_AVRO_SCHEMA_V1)
- .build()?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV1>(&values)?.try_into()
- }
- FormatVersion::V2 => {
- let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV2>(&values)?.try_into()
- }
- FormatVersion::V3 => {
- let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV3>(&values)?.try_into()
- }
- }
+ let reader = Reader::new(bs)?;
+ let entries = match version {
+ FormatVersion::V1 => reader
+ .into_deser_iter::<Resolved<_serde::ManifestFileV1>>()
+ .map(|entry| entry?.0.try_into())
+ .collect::<Result<Vec<_>>>()?,
Review Comment:
Great find, opened https://github.com/apache/iceberg-rust/issues/3445
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] perf: parse manifest list entries without buffering Avro values [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3224:
URL: https://github.com/apache/iceberg-rust/pull/3224#discussion_r4259730524
##########
crates/iceberg/src/spec/manifest_list/mod.rs:
##########
@@ -58,22 +57,39 @@ pub struct ManifestList {
impl ManifestList {
/// Parse manifest list from bytes.
+ /// Convert each Avro value immediately, retaining only the final typed entries.
+ /// A decoding or conversion error still discards the entire result.
pub fn parse_with_version(bs: &[u8], version: FormatVersion) -> Result<ManifestList> {
match version {
FormatVersion::V1 => {
let reader = Reader::with_schema(&MANIFEST_LIST_AVRO_SCHEMA_V1, bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV1>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV1>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
FormatVersion::V2 => {
let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV2>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV2>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
FormatVersion::V3 => {
let reader = Reader::new(bs)?;
- let values = Value::Array(reader.collect::<std::result::Result<Vec<Value>, _>>()?);
- from_value::<_serde::ManifestListV3>(&values)?.try_into()
+ let entries = reader
+ .map(|value| {
+ let value = value?;
+ from_value::<_serde::ManifestFileV3>(&value)?.try_into()
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ManifestList { entries })
}
}
}
Review Comment:
> Done. I merged `main` and use your `into_deser_iter` version. Peak is now 4,355,177 bytes, against 31,194,672 on `main`.
Awesome, and thank you for updating the PR description with the new numbers!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org