Skip to content

Commit b80ebbf

Browse files
authored
fix(writer): validate equality delete field ids (#2723)
## Which issue does this PR close? - Closes #2722. ## What changes are included in this PR? - Add writer config validation for equality delete field IDs. - Reject empty equality ID lists, duplicate equality field IDs, and field IDs that are missing from the table schema. - Keep the existing `RecordBatchProjector` behavior for unsupported field types and map/list reachability. - Fix a stale comment: equality delete fields may be optional, unlike identifier fields. ## Are these changes tested? - `cargo test -p iceberg test_equality_delete_rejects_empty_equality_ids --lib` - `cargo test -p iceberg test_equality_delete_rejects_duplicate_equality_ids --lib` - `cargo test -p iceberg test_equality_delete_rejects_missing_equality_id --lib` - `cargo test -p iceberg test_equality_delete_unreachable_column --lib` - `cargo test -p iceberg test_equality_delete_with_nullable_field --lib` - `cargo test -p iceberg equality_delete --lib` - `cargo fmt --check` - `cargo test -p iceberg --lib` - `cargo clippy -p iceberg --all-features --lib --tests -- -D warnings` - `make check` - `make unit-test` - Docker-backed integration path: - `docker compose -f dev/docker-compose.yaml up -d --build --wait` - `make nextest` (1832 passed, 0 skipped)
1 parent 5627a31 commit b80ebbf

1 file changed

Lines changed: 101 additions & 6 deletions

File tree

crates/iceberg/src/writer/base_writer/equality_delete_writer.rs

Lines changed: 101 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
//! This module provide `EqualityDeleteWriter`.
1919
20+
use std::collections::HashSet;
2021
use std::sync::Arc;
2122

2223
use arrow_array::RecordBatch;
@@ -26,7 +27,7 @@ use parquet::arrow::PARQUET_FIELD_ID_META_KEY;
2627

2728
use crate::arrow::record_batch_projector::RecordBatchProjector;
2829
use crate::arrow::schema_to_arrow_schema;
29-
use crate::spec::{DataFile, PartitionKey, SchemaRef};
30+
use crate::spec::{DataFile, PartitionKey, Schema, SchemaRef};
3031
use crate::writer::file_writer::FileWriterBuilder;
3132
use crate::writer::file_writer::location_generator::{FileNameGenerator, LocationGenerator};
3233
use crate::writer::file_writer::rolling_writer::{RollingFileWriter, RollingFileWriterBuilder};
@@ -68,17 +69,56 @@ pub struct EqualityDeleteWriterConfig {
6869
projector: RecordBatchProjector,
6970
}
7071

72+
/// Validates equality delete field ids before constructing the writer config.
73+
///
74+
/// In addition to rejecting an empty id list, this check also rejects duplicate
75+
/// ids and ids that do not exist in the table schema. These two checks are
76+
/// intentionally stricter than the current Java reference implementation, which
77+
/// only guards against null/empty lists; catching duplicates and missing ids
78+
/// here produces clearer errors and avoids generating meaningless delete files.
79+
fn validate_equality_ids(equality_ids: &[i32], original_schema: &Schema) -> Result<()> {
80+
if equality_ids.is_empty() {
81+
return Err(Error::new(
82+
ErrorKind::DataInvalid,
83+
"Equality delete field ids must not be empty.",
84+
));
85+
}
86+
87+
let mut seen = HashSet::with_capacity(equality_ids.len());
88+
for id in equality_ids {
89+
if !seen.insert(*id) {
90+
return Err(Error::new(
91+
ErrorKind::DataInvalid,
92+
format!("Duplicate equality delete field id: {id}"),
93+
)
94+
.with_context("field_id", id.to_string()));
95+
}
96+
97+
if original_schema.field_by_id(*id).is_none() {
98+
return Err(Error::new(
99+
ErrorKind::DataInvalid,
100+
format!("Invalid equality delete field id: {id}"),
101+
)
102+
.with_context("field_id", id.to_string()));
103+
}
104+
}
105+
106+
Ok(())
107+
}
108+
71109
impl EqualityDeleteWriterConfig {
72-
/// Create a new `DataFileWriterConfig` with equality ids.
110+
/// Create a new `EqualityDeleteWriterConfig` with equality ids.
73111
pub fn new(equality_ids: Vec<i32>, original_schema: SchemaRef) -> Result<Self> {
112+
validate_equality_ids(&equality_ids, &original_schema)?;
113+
74114
let original_arrow_schema = Arc::new(schema_to_arrow_schema(&original_schema)?);
75115
let projector = RecordBatchProjector::new(
76116
original_arrow_schema,
77117
&equality_ids,
78-
// The following rule comes from https://iceberg.apache.org/spec/#identifier-field-ids
79-
// and https://iceberg.apache.org/spec/#equality-delete-files
80-
// - The identifier field ids must be used for primitive types.
81-
// - The identifier field ids must not be used for floating point types or nullable fields.
118+
// Equality delete fields follow identifier-field type restrictions,
119+
// except optional columns and columns nested under optional structs are allowed.
120+
// Project only primitive, non-floating fields; RecordBatchProjector's traversal
121+
// keeps fields under maps and lists unreachable.
82122
|field| {
83123
// Only primitive type is allowed to be used for identifier field ids
84124
if field.data_type().is_nested()
@@ -212,6 +252,7 @@ mod test {
212252
use tempfile::TempDir;
213253
use uuid::Uuid;
214254

255+
use crate::ErrorKind;
215256
use crate::arrow::{arrow_schema_to_schema, schema_to_arrow_schema};
216257
use crate::io::FileIO;
217258
use crate::spec::{
@@ -458,6 +499,60 @@ mod test {
458499
Ok(())
459500
}
460501

502+
fn equality_id_validation_schema() -> Arc<Schema> {
503+
Arc::new(
504+
Schema::builder()
505+
.with_schema_id(1)
506+
.with_fields(vec![
507+
NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),
508+
NestedField::optional(2, "name", Type::Primitive(PrimitiveType::String)).into(),
509+
])
510+
.build()
511+
.unwrap(),
512+
)
513+
}
514+
515+
#[test]
516+
fn test_equality_delete_rejects_empty_equality_ids() {
517+
let err =
518+
EqualityDeleteWriterConfig::new(vec![], equality_id_validation_schema()).unwrap_err();
519+
520+
assert_eq!(err.kind(), ErrorKind::DataInvalid);
521+
assert!(
522+
err.to_string()
523+
.contains("Equality delete field ids must not be empty."),
524+
"{err}"
525+
);
526+
}
527+
528+
#[test]
529+
fn test_equality_delete_rejects_duplicate_equality_ids() {
530+
let err = EqualityDeleteWriterConfig::new(vec![1, 1], equality_id_validation_schema())
531+
.unwrap_err();
532+
533+
assert_eq!(err.kind(), ErrorKind::DataInvalid);
534+
assert!(
535+
err.to_string()
536+
.contains("Duplicate equality delete field id: 1"),
537+
"{err}"
538+
);
539+
assert!(err.to_string().contains("field_id: 1"), "{err}");
540+
}
541+
542+
#[test]
543+
fn test_equality_delete_rejects_missing_equality_id() {
544+
let err =
545+
EqualityDeleteWriterConfig::new(vec![99], equality_id_validation_schema()).unwrap_err();
546+
547+
assert_eq!(err.kind(), ErrorKind::DataInvalid);
548+
assert!(
549+
err.to_string()
550+
.contains("Invalid equality delete field id: 99"),
551+
"{err}"
552+
);
553+
assert!(err.to_string().contains("field_id: 99"), "{err}");
554+
}
555+
461556
#[tokio::test]
462557
async fn test_equality_delete_unreachable_column() -> Result<(), anyhow::Error> {
463558
let schema = Arc::new(

0 commit comments

Comments
 (0)