Skip to content

Commit 84212c1

Browse files
committed
feat: inherit data-file first_row_id on manifest read
Manifest entries inherit snapshot and sequence numbers on read but not first_row_id, leaving the field parse-only. Assign it during load_manifest from the manifest-level first_row_id plus a running record count, skipping DELETED entries and preserving any already-set value. Mirrors Java's ManifestReader.idAssigner. This is the first step toward reading the _row_id metadata column, which is derived from a data file's first_row_id and each row's position. Refs #2879
1 parent 252bb60 commit 84212c1

1 file changed

Lines changed: 245 additions & 45 deletions

File tree

crates/iceberg/src/spec/manifest_list/manifest_file.rs

Lines changed: 245 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use super::ByteBuf;
2323
use crate::encryption::{EncryptedInputFile, StandardKeyMetadata};
2424
use crate::error::Result;
2525
use crate::io::FileIO;
26-
use crate::spec::Manifest;
26+
use crate::spec::{Manifest, ManifestEntry};
2727
use crate::{Error, ErrorKind};
2828

2929
/// Entry in a manifest list.
@@ -195,8 +195,61 @@ impl ManifestFile {
195195
entry.inherit_data(self);
196196
}
197197

198+
self.assign_first_row_ids(&mut entries)?;
199+
198200
Ok(Manifest::new(metadata, entries))
199201
}
202+
203+
/// Assigns `first_row_id` to data-file entries that do not already have one,
204+
/// starting from the manifest-level `first_row_id` and advancing by each
205+
/// entry's record count. See the row-lineage inheritance rules in
206+
/// <https://github.com/apache/iceberg/blob/main/format/spec.md#first-row-id-inheritance>.
207+
fn assign_first_row_ids(&self, entries: &mut [ManifestEntry]) -> Result<()> {
208+
let Some(manifest_first_row_id) = self.first_row_id else {
209+
return Ok(());
210+
};
211+
212+
// A `first_row_id` is only valid on data manifests; delete files always
213+
// have a null `first_row_id`.
214+
if self.content != ManifestContentType::Data {
215+
return Err(Error::new(
216+
ErrorKind::DataInvalid,
217+
format!(
218+
"first_row_id is not valid for delete manifests (found {manifest_first_row_id} on {})",
219+
self.manifest_path
220+
),
221+
));
222+
}
223+
224+
let mut next_row_id = i64::try_from(manifest_first_row_id).map_err(|_| {
225+
Error::new(
226+
ErrorKind::DataInvalid,
227+
format!("Invalid first_row_id: {manifest_first_row_id} (exceeds i64::MAX)"),
228+
)
229+
})?;
230+
231+
for entry in entries {
232+
if !entry.is_alive() {
233+
continue;
234+
}
235+
236+
if entry.data_file.first_row_id.is_none() {
237+
entry.data_file.first_row_id = Some(next_row_id);
238+
let record_count = entry.data_file.record_count;
239+
next_row_id = next_row_id.checked_add_unsigned(record_count).ok_or_else(|| {
240+
Error::new(
241+
ErrorKind::DataInvalid,
242+
format!(
243+
"Row ID overflow assigning first_row_id in {}. Next Row ID: {next_row_id}, Record Count: {record_count}",
244+
self.manifest_path
245+
),
246+
)
247+
})?;
248+
}
249+
}
250+
251+
Ok(())
252+
}
200253
}
201254

202255
/// Field summary for partition field in the spec.
@@ -225,16 +278,15 @@ pub struct FieldSummary {
225278

226279
#[cfg(test)]
227280
mod test {
228-
use std::collections::HashMap;
229281
use std::sync::Arc;
230282

231283
use super::{ManifestContentType, ManifestFile};
232284
use crate::ErrorKind;
233285
use crate::encryption::{EncryptedOutputFile, StandardKeyMetadata};
234286
use crate::io::FileIO;
235287
use crate::spec::{
236-
DataContentType, DataFile, DataFileFormat, ManifestEntry, ManifestStatus,
237-
ManifestWriterBuilder, NestedField, PartitionSpec, PrimitiveType, Schema, Struct, Type,
288+
DataContentType, DataFileBuilder, DataFileFormat, ManifestEntry, ManifestStatus,
289+
ManifestWriterBuilder, NestedField, PartitionSpec, PrimitiveType, Schema, SchemaRef, Type,
238290
};
239291

240292
#[test]
@@ -247,15 +299,9 @@ mod test {
247299
assert_eq!(ManifestContentType::default() as i32, 0);
248300
}
249301

250-
/// Writes a single-entry manifest, encrypting it with `key_metadata`, and
251-
/// returns the resulting [`ManifestFile`]. The manifest is stored in `io`
252-
/// at `path`.
253-
async fn write_encrypted_manifest(
254-
io: &FileIO,
255-
path: &str,
256-
key_metadata: StandardKeyMetadata,
257-
) -> ManifestFile {
258-
let schema = Arc::new(
302+
/// A single-field schema used by the manifest-writing test helpers.
303+
fn test_schema() -> SchemaRef {
304+
Arc::new(
259305
Schema::builder()
260306
.with_fields(vec![Arc::new(NestedField::optional(
261307
1,
@@ -264,8 +310,37 @@ mod test {
264310
))])
265311
.build()
266312
.unwrap(),
267-
);
313+
)
314+
}
268315

316+
/// Writes a single-entry v3 data manifest to `io` at `path`, without
317+
/// encryption, and returns the resulting [`ManifestFile`].
318+
async fn write_manifest(io: &FileIO, path: &str) -> ManifestFile {
319+
let schema = test_schema();
320+
let partition_spec = PartitionSpec::builder(schema.clone())
321+
.with_spec_id(0)
322+
.build()
323+
.unwrap();
324+
325+
let output_file = io.new_output(path).unwrap();
326+
let mut writer = ManifestWriterBuilder::new(output_file, Some(1), schema, partition_spec)
327+
.build_v3_data();
328+
329+
writer
330+
.add_entry(data_entry(ManifestStatus::Added, 100, None))
331+
.unwrap();
332+
333+
writer.write_manifest_file().await.unwrap()
334+
}
335+
336+
/// Writes a single-entry v3 data manifest to `io` at `path`, encrypting it
337+
/// with `key_metadata`, and returns the resulting [`ManifestFile`].
338+
async fn write_encrypted_manifest(
339+
io: &FileIO,
340+
path: &str,
341+
key_metadata: StandardKeyMetadata,
342+
) -> ManifestFile {
343+
let schema = test_schema();
269344
let partition_spec = PartitionSpec::builder(schema.clone())
270345
.with_spec_id(0)
271346
.build()
@@ -277,42 +352,14 @@ mod test {
277352
let mut writer = ManifestWriterBuilder::new_from_encrypted(
278353
encrypted_output,
279354
Some(1),
280-
schema.clone(),
281-
partition_spec.clone(),
355+
schema,
356+
partition_spec,
282357
)
283358
.expect("Expected a valid writer")
284359
.build_v3_data();
285360

286361
writer
287-
.add_entry(ManifestEntry {
288-
status: ManifestStatus::Added,
289-
snapshot_id: None,
290-
sequence_number: None,
291-
file_sequence_number: None,
292-
data_file: DataFile {
293-
content: DataContentType::Data,
294-
file_path: "s3://bucket/table/data/00000.parquet".to_string(),
295-
file_format: DataFileFormat::Parquet,
296-
partition: Struct::empty(),
297-
record_count: 100,
298-
file_size_in_bytes: 4096,
299-
column_sizes: HashMap::new(),
300-
value_counts: HashMap::new(),
301-
null_value_counts: HashMap::new(),
302-
nan_value_counts: HashMap::new(),
303-
lower_bounds: HashMap::new(),
304-
upper_bounds: HashMap::new(),
305-
key_metadata: None,
306-
split_offsets: None,
307-
equality_ids: None,
308-
sort_order_id: None,
309-
partition_spec_id: 0,
310-
first_row_id: None,
311-
referenced_data_file: None,
312-
content_offset: None,
313-
content_size_in_bytes: None,
314-
},
315-
})
362+
.add_entry(data_entry(ManifestStatus::Added, 100, None))
316363
.unwrap();
317364

318365
writer.write_manifest_file().await.unwrap()
@@ -389,4 +436,157 @@ mod test {
389436
.expect_err("load_manifest must fail when decrypting with the wrong AAD prefix");
390437
assert_eq!(err.kind(), ErrorKind::Unexpected);
391438
}
439+
440+
/// Builds a data-file manifest entry with the given status, record count,
441+
/// and pre-existing `first_row_id`.
442+
fn data_entry(
443+
status: ManifestStatus,
444+
record_count: u64,
445+
first_row_id: Option<i64>,
446+
) -> ManifestEntry {
447+
let mut builder = DataFileBuilder::default();
448+
builder
449+
.content(DataContentType::Data)
450+
.file_path("s3://bucket/table/data/00000.parquet".to_string())
451+
.file_format(DataFileFormat::Parquet)
452+
.file_size_in_bytes(4096)
453+
.record_count(record_count);
454+
if let Some(id) = first_row_id {
455+
builder.first_row_id(Some(id));
456+
}
457+
458+
ManifestEntry::builder()
459+
.status(status)
460+
.data_file(builder.build().unwrap())
461+
.build()
462+
}
463+
464+
/// Builds a manifest file with the given content type and manifest-level
465+
/// `first_row_id`. Other fields are irrelevant to row-id assignment.
466+
fn manifest_file(content: ManifestContentType, first_row_id: Option<u64>) -> ManifestFile {
467+
ManifestFile {
468+
manifest_path: "memory:///m.avro".to_string(),
469+
manifest_length: 0,
470+
partition_spec_id: 0,
471+
content,
472+
sequence_number: 0,
473+
min_sequence_number: 0,
474+
added_snapshot_id: 0,
475+
added_files_count: None,
476+
existing_files_count: None,
477+
deleted_files_count: None,
478+
added_rows_count: None,
479+
existing_rows_count: None,
480+
deleted_rows_count: None,
481+
partitions: None,
482+
key_metadata: None,
483+
first_row_id,
484+
}
485+
}
486+
487+
#[test]
488+
fn test_assign_first_row_ids_interleaved() {
489+
let manifest = manifest_file(ManifestContentType::Data, Some(10));
490+
let mut entries = vec![
491+
data_entry(ManifestStatus::Added, 3, None),
492+
// A pre-assigned entry between two assigned ones: it keeps its id and
493+
// must not advance the running counter.
494+
data_entry(ManifestStatus::Added, 5, Some(100)),
495+
// A deleted entry likewise neither receives nor consumes a row id.
496+
data_entry(ManifestStatus::Deleted, 7, None),
497+
data_entry(ManifestStatus::Existing, 2, None),
498+
];
499+
500+
manifest.assign_first_row_ids(&mut entries).unwrap();
501+
502+
assert_eq!(entries[0].data_file.first_row_id, Some(10));
503+
assert_eq!(entries[1].data_file.first_row_id, Some(100));
504+
assert_eq!(entries[2].data_file.first_row_id, None);
505+
// 10 + 3 = 13; the preserved and deleted entries in between do not move it.
506+
assert_eq!(entries[3].data_file.first_row_id, Some(13));
507+
}
508+
509+
#[test]
510+
fn test_assign_first_row_ids_noop_without_manifest_first_row_id() {
511+
let manifest = manifest_file(ManifestContentType::Data, None);
512+
let mut entries = vec![data_entry(ManifestStatus::Added, 3, None)];
513+
514+
manifest.assign_first_row_ids(&mut entries).unwrap();
515+
516+
assert_eq!(entries[0].data_file.first_row_id, None);
517+
}
518+
519+
#[test]
520+
fn test_assign_first_row_ids_rejects_delete_manifest() {
521+
let manifest = manifest_file(ManifestContentType::Deletes, Some(10));
522+
let mut entries = vec![data_entry(ManifestStatus::Added, 3, None)];
523+
524+
let err = manifest
525+
.assign_first_row_ids(&mut entries)
526+
.expect_err("a delete manifest with a first_row_id must be rejected");
527+
assert_eq!(err.kind(), ErrorKind::DataInvalid);
528+
assert!(err.message().contains("delete manifests"));
529+
}
530+
531+
#[test]
532+
fn test_assign_first_row_ids_rejects_oversized_manifest_first_row_id() {
533+
// A manifest-level first_row_id above i64::MAX cannot be represented as the
534+
// signed running counter and must be rejected.
535+
let manifest = manifest_file(ManifestContentType::Data, Some(i64::MAX as u64 + 1));
536+
let mut entries = vec![data_entry(ManifestStatus::Added, 3, None)];
537+
538+
let err = manifest
539+
.assign_first_row_ids(&mut entries)
540+
.expect_err("an oversized manifest first_row_id must be rejected");
541+
assert_eq!(err.kind(), ErrorKind::DataInvalid);
542+
assert!(err.message().contains("Invalid first_row_id"));
543+
}
544+
545+
#[test]
546+
fn test_assign_first_row_ids_rejects_counter_overflow() {
547+
// Advancing the running counter past i64::MAX must be rejected rather than
548+
// wrapping to a negative value that would corrupt subsequent assignments.
549+
let manifest = manifest_file(ManifestContentType::Data, Some(i64::MAX as u64));
550+
let mut entries = vec![data_entry(ManifestStatus::Added, 1, None)];
551+
552+
let err = manifest
553+
.assign_first_row_ids(&mut entries)
554+
.expect_err("counter overflow past i64::MAX must be rejected");
555+
assert_eq!(err.kind(), ErrorKind::DataInvalid);
556+
assert!(err.message().contains("Row ID overflow"));
557+
}
558+
559+
#[tokio::test]
560+
async fn test_load_manifest_reads_written_entries() {
561+
let io = FileIO::new_with_memory();
562+
let path = "memory:///test/plaintext_manifest.avro";
563+
let manifest_file = write_manifest(&io, path).await;
564+
assert_eq!(manifest_file.key_metadata, None);
565+
566+
let manifest = manifest_file.load_manifest(&io).await.unwrap();
567+
assert_eq!(manifest.entries().len(), 1);
568+
assert_eq!(
569+
manifest.entries()[0].file_path(),
570+
"s3://bucket/table/data/00000.parquet"
571+
);
572+
assert_eq!(manifest.entries()[0].data_file.record_count, 100);
573+
}
574+
575+
/// End-to-end: writing a v3 data manifest, stamping a manifest-level
576+
/// `first_row_id`, and loading it must assign inherited `first_row_id`s to
577+
/// the entries. This exercises the wiring in `load_manifest` and the
578+
/// write/read round-trip that leaves per-file `first_row_id` as `None`.
579+
#[tokio::test]
580+
async fn test_load_manifest_assigns_first_row_ids() {
581+
let io = FileIO::new_with_memory();
582+
let path = "memory:///test/first_row_id_manifest.avro";
583+
let mut manifest_file = write_manifest(&io, path).await;
584+
585+
// Stamp a manifest-level first_row_id, as the manifest-list writer would.
586+
manifest_file.first_row_id = Some(1000);
587+
588+
let manifest = manifest_file.load_manifest(&io).await.unwrap();
589+
assert_eq!(manifest.entries().len(), 1);
590+
assert_eq!(manifest.entries()[0].data_file().first_row_id(), Some(1000));
591+
}
392592
}

0 commit comments

Comments
 (0)