laskoviymishka commented on code in PR #3397: URL: https://github.com/apache/iceberg-rust/pull/3397#discussion_r4286660685
########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] Review Comment: I'd limit this allow to the items that are actually unused, or at least point it at #3305 so it's clear when it can go. As written, it will hide anything in here that never gets a caller. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], Review Comment: Later milestones build on this signature, so I'd give the manifest evidence a small named type instead of `(i32, &StructType)`. While we're here, I'd split `new` into three steps: collecting definitions, validating evidence, and resolving types. Indexing each spec's fields by id up front would also remove the `iter().any`/`iter().find` scans per field per manifest. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields Review Comment: My gut feeling is that this makes our stats `partition` struct diverge from Java's, but I haven't verified it. Can you check? As I remember it, `Partitioning.partitionType(table)` (what `PartitionStatsHandler` builds on) goes through `allActiveFieldIds` and drops fields whose source isn't in the current schema, the same way our `compute_unified_partition_type` does. If that holds, we write extra columns. Since `mapping()` requires every non-void field, a Java-written stats file in a table with a dropped partition source would then fail with `Missing necessary partition field`. If it does diverge, we can either match Java, or keep the superset on purpose and have `mapping()` accept missing dropped-source fields when the layout comes from a stats file rather than a manifest. Either works, but I'd like the choice written down, with an interop test against a Java-shaped layout. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { Review Comment: I'd make `equivalent_ignoring_names`, `compatible_transforms` and `is_void_transform` in the parent module `pub(super)` and call them here, instead of writing the rule out again. The same applies to the `== Transform::Void` checks further down. With two copies of what makes field definitions compatible, the next v1 quirk fix will land in one and not the other. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { + if existing.source_id != field.source_id + || (existing.transform != Transform::Void + && field.transform != Transform::Void + && existing.transform != field.transform) + { + return Err(invalid_data!( + "Conflicting definitions for partition field {}", + field.field_id + )); + } + // Keep the newest name, but use the historical non-void transform. + if existing.transform == Transform::Void { + existing.transform = field.transform; + } + } else { + definitions.insert(field.field_id, field.clone()); + } + } + } + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id + .get(spec_id) + .ok_or_else(|| invalid_data!("Unknown manifest partition spec ID {spec_id}"))?; + physical_positions(physical_type)?; + for field in physical_type.fields() { + if !spec + .fields() + .iter() + .any(|definition| definition.field_id == field.id) + { + return Err(invalid_data!( + "Manifest partition field {} is absent from spec {spec_id}", + field.id + )); + } + } + } + + let mut fields = Vec::with_capacity(definitions.len()); + for (id, definition) in &definitions { + let mut source_type = None; + for schema in schemas { + if let Some(source) = schema.field_by_id(definition.source_id) + && *source.field_type != Type::Primitive(PrimitiveType::Unknown) + { + merge_type( + &mut source_type, + concrete_type(&source.field_type, *id)?, + *id, + )?; + } + } + let has_source_type = source_type.is_some(); + let mut output_type = match source_type { + Some(source) => Some(concrete_type( + &definition.transform.result_type(&source.into())?, + *id, + )?), + None => fixed_result_type(definition.transform), + }; + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id[spec_id]; Review Comment: This index panics if the check in the earlier manifest loop ever stops holding. I'd reuse the spec looked up there, or use `.get()` with `ok_or_else`. The other index sites (246, 278, 291, 330, 358) are the same: each is safe today only because of an invariant set up somewhere else, and we've been keeping library paths panic-free. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { + if existing.source_id != field.source_id + || (existing.transform != Transform::Void + && field.transform != Transform::Void + && existing.transform != field.transform) + { + return Err(invalid_data!( + "Conflicting definitions for partition field {}", + field.field_id + )); + } + // Keep the newest name, but use the historical non-void transform. + if existing.transform == Transform::Void { + existing.transform = field.transform; + } + } else { + definitions.insert(field.field_id, field.clone()); + } + } + } + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id + .get(spec_id) + .ok_or_else(|| invalid_data!("Unknown manifest partition spec ID {spec_id}"))?; + physical_positions(physical_type)?; + for field in physical_type.fields() { + if !spec + .fields() + .iter() + .any(|definition| definition.field_id == field.id) + { + return Err(invalid_data!( + "Manifest partition field {} is absent from spec {spec_id}", + field.id + )); + } + } + } + + let mut fields = Vec::with_capacity(definitions.len()); + for (id, definition) in &definitions { + let mut source_type = None; + for schema in schemas { + if let Some(source) = schema.field_by_id(definition.source_id) + && *source.field_type != Type::Primitive(PrimitiveType::Unknown) + { + merge_type( Review Comment: This fails on a v3 table the spec allows. v3 permits `date` → `timestamp`/`timestamp_ns`, and allows it for a partition source when the transform's result doesn't change, which covers `day`/`month`/`year`. After that promotion, the schemas give `Date` and `Timestamp` for the same source id. `can_promote` doesn't accept that pair, so `new` fails with `Conflicting types` even though the output type is the same either way. I'd teach the source-type merge specifically about date → timestamp(_ns), not `can_promote` in general. `mapping()` and `normalize` also use `can_promote` and have no days→micros conversion. That's fine, because the spec doesn't allow identity or bucket over a promoted date. A `day(d)` test with `d` promoted would cover this. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { + if existing.source_id != field.source_id + || (existing.transform != Transform::Void + && field.transform != Transform::Void + && existing.transform != field.transform) + { + return Err(invalid_data!( + "Conflicting definitions for partition field {}", + field.field_id + )); + } + // Keep the newest name, but use the historical non-void transform. + if existing.transform == Transform::Void { + existing.transform = field.transform; + } + } else { + definitions.insert(field.field_id, field.clone()); + } + } + } + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id + .get(spec_id) + .ok_or_else(|| invalid_data!("Unknown manifest partition spec ID {spec_id}"))?; + physical_positions(physical_type)?; + for field in physical_type.fields() { + if !spec + .fields() + .iter() + .any(|definition| definition.field_id == field.id) + { + return Err(invalid_data!( + "Manifest partition field {} is absent from spec {spec_id}", + field.id + )); + } + } + } + + let mut fields = Vec::with_capacity(definitions.len()); + for (id, definition) in &definitions { + let mut source_type = None; + for schema in schemas { + if let Some(source) = schema.field_by_id(definition.source_id) + && *source.field_type != Type::Primitive(PrimitiveType::Unknown) + { + merge_type( + &mut source_type, + concrete_type(&source.field_type, *id)?, + *id, + )?; + } + } + let has_source_type = source_type.is_some(); + let mut output_type = match source_type { + Some(source) => Some(concrete_type( + &definition.transform.result_type(&source.into())?, + *id, + )?), + None => fixed_result_type(definition.transform), + }; + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id[spec_id]; + let Some(spec_field) = spec.fields().iter().find(|field| field.field_id == *id) + else { + continue; + }; + // A void field stores only nulls; a writer's placeholder type cannot + // replace a historical non-void type or a known source type. + if spec_field.transform == Transform::Void + && (definition.transform != Transform::Void || has_source_type) + { + continue; + } + if let Some(physical) = physical_type.fields().iter().find(|field| field.id == *id) + && *physical.field_type != Type::Primitive(PrimitiveType::Unknown) + { + let candidate = concrete_type(&physical.field_type, *id)?; + if fixed_result_type(definition.transform) + .is_some_and(|expected| expected != candidate) + { + return Err(invalid_data!( + "Invalid physical result type for partition field {id}: {candidate:?}" + )); + } + // Identity and truncate preserve the source type. Validate that + // physical evidence is a possible output of the transform. + if matches!( + definition.transform, + Transform::Identity | Transform::Truncate(_) + ) { + definition + .transform + .result_type(&candidate.clone().into())?; + } + merge_type(&mut output_type, candidate, *id)?; + } + } + let ty = output_type + .or_else(|| (definition.transform == Transform::Void).then_some(PrimitiveType::Int)) Review Comment: `Int` here isn't tied to anything: there's no source and no evidence. If Java does leave dropped-source fields out (see the question above), this also gives our stats file a column other clients don't have. I'd either drop the field or add a comment saying why `Int` was chosen. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { + if existing.source_id != field.source_id + || (existing.transform != Transform::Void + && field.transform != Transform::Void + && existing.transform != field.transform) + { + return Err(invalid_data!( + "Conflicting definitions for partition field {}", + field.field_id + )); + } + // Keep the newest name, but use the historical non-void transform. + if existing.transform == Transform::Void { + existing.transform = field.transform; + } + } else { + definitions.insert(field.field_id, field.clone()); + } + } + } + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id + .get(spec_id) + .ok_or_else(|| invalid_data!("Unknown manifest partition spec ID {spec_id}"))?; + physical_positions(physical_type)?; + for field in physical_type.fields() { + if !spec + .fields() + .iter() + .any(|definition| definition.field_id == field.id) + { + return Err(invalid_data!( + "Manifest partition field {} is absent from spec {spec_id}", + field.id + )); + } + } + } + + let mut fields = Vec::with_capacity(definitions.len()); + for (id, definition) in &definitions { + let mut source_type = None; + for schema in schemas { + if let Some(source) = schema.field_by_id(definition.source_id) + && *source.field_type != Type::Primitive(PrimitiveType::Unknown) + { + merge_type( + &mut source_type, + concrete_type(&source.field_type, *id)?, + *id, + )?; + } + } + let has_source_type = source_type.is_some(); + let mut output_type = match source_type { + Some(source) => Some(concrete_type( + &definition.transform.result_type(&source.into())?, + *id, + )?), + None => fixed_result_type(definition.transform), + }; + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id[spec_id]; + let Some(spec_field) = spec.fields().iter().find(|field| field.field_id == *id) + else { + continue; + }; + // A void field stores only nulls; a writer's placeholder type cannot + // replace a historical non-void type or a known source type. + if spec_field.transform == Transform::Void + && (definition.transform != Transform::Void || has_source_type) + { + continue; + } + if let Some(physical) = physical_type.fields().iter().find(|field| field.id == *id) + && *physical.field_type != Type::Primitive(PrimitiveType::Unknown) + { + let candidate = concrete_type(&physical.field_type, *id)?; + if fixed_result_type(definition.transform) + .is_some_and(|expected| expected != candidate) + { + return Err(invalid_data!( + "Invalid physical result type for partition field {id}: {candidate:?}" + )); + } + // Identity and truncate preserve the source type. Validate that + // physical evidence is a possible output of the transform. + if matches!( + definition.transform, + Transform::Identity | Transform::Truncate(_) + ) { + definition + .transform + .result_type(&candidate.clone().into())?; + } + merge_type(&mut output_type, candidate, *id)?; + } + } + let ty = output_type + .or_else(|| (definition.transform == Transform::Void).then_some(PrimitiveType::Int)) + .ok_or_else(|| { + invalid_data!( + "Cannot resolve historical type for partition field {id} (source {})", + definition.source_id + ) + })?; + fields.push(NestedField::optional(*id, &definition.name, ty.into()).into()); + } + + let partition_type = StructType::new(fields); + let positions = physical_positions(&partition_type)?; + let specs = ordered_specs + .into_iter() + .map(|spec| { + ( + spec.spec_id(), + spec.fields() + .iter() + .map(|field| SpecField { + id: field.field_id, + unified_position: positions[&field.field_id], + is_void: field.transform == Transform::Void, + }) + .collect(), + ) + }) + .collect(); + Ok(Self { + partition_type, + specs, + }) + } + + pub(crate) fn partition_type(&self) -> &StructType { + &self.partition_type + } + + /// Compiles a field-ID mapping from the original physical layout, before + /// projection or default filling. Missing non-void fields are errors even if + /// a decoder would supply nulls; omitted fields outside this spec are safe. + pub(crate) fn mapping( + &self, + spec_id: i32, + physical_type: &StructType, + ) -> Result<StatisticsPartitionMapping> { + let spec = self.spec(spec_id)?; + let positions = physical_positions(physical_type)?; + let fields = spec + .iter() + .map(|field| { + let position = positions.get(&field.id).copied(); + let target_type = concrete_type( + &self.partition_type.fields()[field.unified_position].field_type, + field.id, + )?; + let source_type = if field.is_void { + None + } else { + let position = position.ok_or_else(|| { + invalid_data!( + "Missing necessary partition field {} for spec {spec_id}", + field.id + ) + })?; + let source = + concrete_type(&physical_type.fields()[position].field_type, field.id)?; + if !can_promote(&source, &target_type) { + return Err(invalid_data!( + "Cannot promote partition field {} from {source:?} to {target_type:?}", + field.id + )); + } + Some(source) + }; + Ok(ValueMapping { + id: field.id, + position, + source_type, + target_type, + is_void: field.is_void, + }) + }) + .collect::<Result<_>>()?; + Ok(StatisticsPartitionMapping { + spec_id, + physical_len: physical_type.fields().len(), + fields, + }) + } + + /// Projects a canonical per-spec key into the full optional partition layout. + pub(crate) fn unified_partition(&self, key: &StatisticsPartitionKey) -> Result<Struct> { + let spec = self.spec(key.spec_id)?; + check_tuple_len(&key.partition, spec.len())?; + let mut values = vec![None; self.partition_type.fields().len()]; + for (field, value) in spec.iter().zip(key.partition.iter()) { + if let Some(value) = value { + if field.is_void { + return Err(invalid_data!( + "Non-null value for void partition field {}", + field.id + )); + } + let ty = concrete_type( + &self.partition_type.fields()[field.unified_position].field_type, + field.id, + )?; + values[field.unified_position] = Some(normalize(value, &ty, &ty, field.id)?); + } + } + Ok(values.into_iter().collect()) + } + + fn spec(&self, id: i32) -> Result<&[SpecField]> { + self.specs + .get(&id) + .map(Vec::as_slice) + .ok_or_else(|| invalid_data!("Unknown partition spec ID {id}")) + } +} + +impl StatisticsPartitionMapping { + /// Recovers original spec order and canonicalizes values before equality/hashing. + /// Void fields are always null and can be recovered without physical storage. + pub(crate) fn key(&self, physical_partition: &Struct) -> Result<StatisticsPartitionKey> { + check_tuple_len(physical_partition, self.physical_len)?; + let partition = self + .fields + .iter() + .map(|field| { + let value = field + .position + .and_then(|position| physical_partition[position].as_ref()); + if field.is_void { + if value.is_some() { + return Err(invalid_data!( + "Non-null value for void partition field {}", + field.id + )); + } + return Ok(None); + } + value + .map(|value| { + normalize( + value, + field + .source_type + .as_ref() + .expect("Non-void mappings have a source type"), Review Comment: I'd model void vs. value as an enum so this `expect` can't fire, instead of relying on `mapping()` to keep `is_void`, `source_type` and `position` in sync: ```rust enum ValueMapping { Void { id: i32, position: Option<usize> }, Value { id: i32, position: usize, source_type: PrimitiveType, target_type: PrimitiveType }, } ``` That removes the `expect`, the `is_void` flag, and the `Option` on a position that `mapping()` already requires. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { + if existing.source_id != field.source_id + || (existing.transform != Transform::Void + && field.transform != Transform::Void + && existing.transform != field.transform) + { + return Err(invalid_data!( + "Conflicting definitions for partition field {}", + field.field_id + )); + } + // Keep the newest name, but use the historical non-void transform. + if existing.transform == Transform::Void { + existing.transform = field.transform; + } + } else { + definitions.insert(field.field_id, field.clone()); + } + } + } + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id + .get(spec_id) + .ok_or_else(|| invalid_data!("Unknown manifest partition spec ID {spec_id}"))?; + physical_positions(physical_type)?; + for field in physical_type.fields() { + if !spec + .fields() + .iter() + .any(|definition| definition.field_id == field.id) + { + return Err(invalid_data!( + "Manifest partition field {} is absent from spec {spec_id}", + field.id + )); + } + } + } + + let mut fields = Vec::with_capacity(definitions.len()); + for (id, definition) in &definitions { + let mut source_type = None; + for schema in schemas { + if let Some(source) = schema.field_by_id(definition.source_id) + && *source.field_type != Type::Primitive(PrimitiveType::Unknown) + { + merge_type( + &mut source_type, + concrete_type(&source.field_type, *id)?, + *id, + )?; + } + } + let has_source_type = source_type.is_some(); + let mut output_type = match source_type { + Some(source) => Some(concrete_type( + &definition.transform.result_type(&source.into())?, + *id, + )?), + None => fixed_result_type(definition.transform), + }; + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id[spec_id]; + let Some(spec_field) = spec.fields().iter().find(|field| field.field_id == *id) + else { + continue; + }; + // A void field stores only nulls; a writer's placeholder type cannot + // replace a historical non-void type or a known source type. + if spec_field.transform == Transform::Void + && (definition.transform != Transform::Void || has_source_type) + { + continue; + } + if let Some(physical) = physical_type.fields().iter().find(|field| field.id == *id) + && *physical.field_type != Type::Primitive(PrimitiveType::Unknown) + { + let candidate = concrete_type(&physical.field_type, *id)?; + if fixed_result_type(definition.transform) + .is_some_and(|expected| expected != candidate) + { + return Err(invalid_data!( + "Invalid physical result type for partition field {id}: {candidate:?}" + )); + } + // Identity and truncate preserve the source type. Validate that + // physical evidence is a possible output of the transform. + if matches!( Review Comment: When the source type is known, this only checks that the transform accepts the physical type, not that the physical type fits the source. So with an `int` source, a manifest that claims `long` quietly widens the stats column to `long`. When `has_source_type`, I'd require the candidate to promote to the source-derived type. That also gives us the missing "physical wider than schema" negative test. ########## crates/iceberg/src/partitioning/statistics.rs: ########## @@ -0,0 +1,522 @@ +// 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. + +//! Historical partition types and field-ID mappings for partition statistics. + +// Kept internal until the statistics codec and computation consume these helpers. +#![allow(dead_code)] + +use std::collections::{BTreeMap, HashMap}; + +use crate::Result; +use crate::error::invalid_data; +use crate::spec::{ + Literal, NestedField, PartitionField, PartitionSpec, PrimitiveLiteral, PrimitiveType, Schema, + Struct, StructType, Transform, Type, +}; + +/// A full historical partition type and the original field order of each spec. +/// +/// Unlike the active-field projection used by ordinary scans, this retains fields +/// whose source columns have been dropped. It performs no storage access. +#[derive(Debug)] +pub(crate) struct StatisticsPartitioning { + partition_type: StructType, + specs: HashMap<i32, Vec<SpecField>>, +} + +#[derive(Debug)] +struct SpecField { + id: i32, + unified_position: usize, + is_void: bool, +} + +/// A key in original spec order, with values promoted to the full historical type. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct StatisticsPartitionKey { + spec_id: i32, + partition: Struct, +} + +impl StatisticsPartitionKey { + pub(crate) fn spec_id(&self) -> i32 { + self.spec_id + } + + pub(crate) fn partition(&self) -> &Struct { + &self.partition + } +} + +/// A reusable plan for recovering one spec's keys from a physical partition layout. +#[derive(Debug)] +pub(crate) struct StatisticsPartitionMapping { + spec_id: i32, + physical_len: usize, + fields: Vec<ValueMapping>, +} + +#[derive(Debug)] +struct ValueMapping { + id: i32, + position: Option<usize>, + source_type: Option<PrimitiveType>, + target_type: PrimitiveType, + is_void: bool, +} + +impl StatisticsPartitioning { + /// Resolves types from known specs, current/retained schemas, and optional + /// physical manifest partition types paired with their spec IDs. + /// + /// Manifest types must come from the physical writer schema, rather than the + /// embedded table schema. The caller owns extraction of that evidence. + pub(crate) fn new( + specs: &[&PartitionSpec], + schemas: &[&Schema], + manifest_types: &[(i32, &StructType)], + ) -> Result<Self> { + let mut ordered_specs = specs.to_vec(); + ordered_specs.sort_by_key(|spec| std::cmp::Reverse(spec.spec_id())); + let mut specs_by_id = HashMap::new(); + let mut definitions: BTreeMap<i32, PartitionField> = BTreeMap::new(); + for spec in &ordered_specs { + if specs_by_id.insert(spec.spec_id(), *spec).is_some() { + return Err(invalid_data!( + "Duplicate partition spec ID {}", + spec.spec_id() + )); + } + let mut seen = std::collections::HashSet::new(); + for field in spec.fields() { + if !seen.insert(field.field_id) { + return Err(invalid_data!( + "Duplicate partition field ID {} in spec {}", + field.field_id, + spec.spec_id() + )); + } + if matches!( + field.transform, + Transform::Unknown | Transform::Bucket(0) | Transform::Truncate(0) + ) { + return Err(invalid_data!( + "Invalid transform {} for partition field {}", + field.transform, + field.field_id + )); + } + if let Some(existing) = definitions.get_mut(&field.field_id) { + if existing.source_id != field.source_id + || (existing.transform != Transform::Void + && field.transform != Transform::Void + && existing.transform != field.transform) + { + return Err(invalid_data!( + "Conflicting definitions for partition field {}", + field.field_id + )); + } + // Keep the newest name, but use the historical non-void transform. + if existing.transform == Transform::Void { + existing.transform = field.transform; + } + } else { + definitions.insert(field.field_id, field.clone()); + } + } + } + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id + .get(spec_id) + .ok_or_else(|| invalid_data!("Unknown manifest partition spec ID {spec_id}"))?; + physical_positions(physical_type)?; + for field in physical_type.fields() { + if !spec + .fields() + .iter() + .any(|definition| definition.field_id == field.id) + { + return Err(invalid_data!( + "Manifest partition field {} is absent from spec {spec_id}", + field.id + )); + } + } + } + + let mut fields = Vec::with_capacity(definitions.len()); + for (id, definition) in &definitions { + let mut source_type = None; + for schema in schemas { + if let Some(source) = schema.field_by_id(definition.source_id) + && *source.field_type != Type::Primitive(PrimitiveType::Unknown) + { + merge_type( + &mut source_type, + concrete_type(&source.field_type, *id)?, + *id, + )?; + } + } + let has_source_type = source_type.is_some(); + let mut output_type = match source_type { + Some(source) => Some(concrete_type( + &definition.transform.result_type(&source.into())?, + *id, + )?), + None => fixed_result_type(definition.transform), + }; + + for (spec_id, physical_type) in manifest_types { + let spec = specs_by_id[spec_id]; + let Some(spec_field) = spec.fields().iter().find(|field| field.field_id == *id) + else { + continue; + }; + // A void field stores only nulls; a writer's placeholder type cannot + // replace a historical non-void type or a known source type. + if spec_field.transform == Transform::Void + && (definition.transform != Transform::Void || has_source_type) + { + continue; + } + if let Some(physical) = physical_type.fields().iter().find(|field| field.id == *id) + && *physical.field_type != Type::Primitive(PrimitiveType::Unknown) + { + let candidate = concrete_type(&physical.field_type, *id)?; + if fixed_result_type(definition.transform) + .is_some_and(|expected| expected != candidate) + { + return Err(invalid_data!( + "Invalid physical result type for partition field {id}: {candidate:?}" + )); + } + // Identity and truncate preserve the source type. Validate that + // physical evidence is a possible output of the transform. + if matches!( + definition.transform, + Transform::Identity | Transform::Truncate(_) + ) { + definition + .transform + .result_type(&candidate.clone().into())?; + } + merge_type(&mut output_type, candidate, *id)?; + } + } + let ty = output_type + .or_else(|| (definition.transform == Transform::Void).then_some(PrimitiveType::Int)) + .ok_or_else(|| { + invalid_data!( + "Cannot resolve historical type for partition field {id} (source {})", + definition.source_id + ) + })?; + fields.push(NestedField::optional(*id, &definition.name, ty.into()).into()); + } + + let partition_type = StructType::new(fields); + let positions = physical_positions(&partition_type)?; + let specs = ordered_specs + .into_iter() + .map(|spec| { + ( + spec.spec_id(), + spec.fields() + .iter() + .map(|field| SpecField { + id: field.field_id, + unified_position: positions[&field.field_id], + is_void: field.transform == Transform::Void, + }) + .collect(), + ) + }) + .collect(); + Ok(Self { + partition_type, + specs, + }) + } + + pub(crate) fn partition_type(&self) -> &StructType { + &self.partition_type + } + + /// Compiles a field-ID mapping from the original physical layout, before + /// projection or default filling. Missing non-void fields are errors even if + /// a decoder would supply nulls; omitted fields outside this spec are safe. + pub(crate) fn mapping( + &self, + spec_id: i32, + physical_type: &StructType, + ) -> Result<StatisticsPartitionMapping> { + let spec = self.spec(spec_id)?; + let positions = physical_positions(physical_type)?; + let fields = spec + .iter() + .map(|field| { + let position = positions.get(&field.id).copied(); + let target_type = concrete_type( + &self.partition_type.fields()[field.unified_position].field_type, + field.id, + )?; + let source_type = if field.is_void { + None + } else { + let position = position.ok_or_else(|| { + invalid_data!( + "Missing necessary partition field {} for spec {spec_id}", + field.id + ) + })?; + let source = + concrete_type(&physical_type.fields()[position].field_type, field.id)?; + if !can_promote(&source, &target_type) { + return Err(invalid_data!( + "Cannot promote partition field {} from {source:?} to {target_type:?}", + field.id + )); + } + Some(source) + }; + Ok(ValueMapping { + id: field.id, + position, + source_type, + target_type, + is_void: field.is_void, + }) + }) + .collect::<Result<_>>()?; + Ok(StatisticsPartitionMapping { + spec_id, + physical_len: physical_type.fields().len(), + fields, + }) + } + + /// Projects a canonical per-spec key into the full optional partition layout. + pub(crate) fn unified_partition(&self, key: &StatisticsPartitionKey) -> Result<Struct> { + let spec = self.spec(key.spec_id)?; + check_tuple_len(&key.partition, spec.len())?; + let mut values = vec![None; self.partition_type.fields().len()]; + for (field, value) in spec.iter().zip(key.partition.iter()) { + if let Some(value) = value { + if field.is_void { + return Err(invalid_data!( + "Non-null value for void partition field {}", + field.id + )); + } + let ty = concrete_type( + &self.partition_type.fields()[field.unified_position].field_type, + field.id, + )?; + values[field.unified_position] = Some(normalize(value, &ty, &ty, field.id)?); + } + } + Ok(values.into_iter().collect()) + } + + fn spec(&self, id: i32) -> Result<&[SpecField]> { + self.specs + .get(&id) + .map(Vec::as_slice) + .ok_or_else(|| invalid_data!("Unknown partition spec ID {id}")) + } +} + +impl StatisticsPartitionMapping { + /// Recovers original spec order and canonicalizes values before equality/hashing. + /// Void fields are always null and can be recovered without physical storage. + pub(crate) fn key(&self, physical_partition: &Struct) -> Result<StatisticsPartitionKey> { + check_tuple_len(physical_partition, self.physical_len)?; + let partition = self + .fields + .iter() + .map(|field| { + let value = field + .position + .and_then(|position| physical_partition[position].as_ref()); + if field.is_void { + if value.is_some() { + return Err(invalid_data!( + "Non-null value for void partition field {}", + field.id + )); + } + return Ok(None); + } + value + .map(|value| { + normalize( + value, + field + .source_type + .as_ref() + .expect("Non-void mappings have a source type"), + &field.target_type, + field.id, + ) + }) + .transpose() + }) + .collect::<Result<Struct>>()?; + Ok(StatisticsPartitionKey { + spec_id: self.spec_id, + partition, + }) + } +} + +fn physical_positions(ty: &StructType) -> Result<HashMap<i32, usize>> { + let mut positions = HashMap::new(); + for (position, field) in ty.fields().iter().enumerate() { + if positions.insert(field.id, position).is_some() { + return Err(invalid_data!( + "Duplicate physical partition field ID {}", + field.id + )); + } + } + Ok(positions) +} + +fn concrete_type(ty: &Type, field_id: i32) -> Result<PrimitiveType> { + let Type::Primitive(primitive) = ty else { + return Err(invalid_data!( + "Non-primitive type for partition field {field_id}: {ty}" + )); + }; + if matches!(primitive, PrimitiveType::Unknown) { + return Err(invalid_data!("Unknown type for partition field {field_id}")); + } + if let PrimitiveType::Decimal { precision, scale } = primitive + && (*precision == 0 || *precision > 38 || scale > precision) Review Comment: We already have `crate::spec::MAX_DECIMAL_PRECISION`; I'd use that instead of the literal `38`. ########## crates/iceberg/src/partitioning/statistics/tests.rs: ########## @@ -0,0 +1,646 @@ +// 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. + +use std::collections::HashSet; + +use super::StatisticsPartitioning; +use crate::ErrorKind; +use crate::spec::{Literal, NestedField, PartitionSpec, PrimitiveType, Schema, Struct, StructType}; + +fn schema(fields: Vec<(i32, PrimitiveType)>) -> Schema { + Schema::builder() + .with_fields( + fields.into_iter().map(|(id, ty)| { + NestedField::optional(id, format!("source_{id}"), ty.into()).into() + }), + ) + .build() + .unwrap() +} + +fn spec(id: i32, fields: Vec<(i32, i32, &str, &str)>) -> PartitionSpec { + serde_json::from_value(serde_json::json!({ + "spec-id": id, + "fields": fields.into_iter().map(|(source, field, name, transform)| { + serde_json::json!({ + "source-id": source, "field-id": field, + "name": name, "transform": transform, + }) + }).collect::<Vec<_>>() + })) + .unwrap() +} + +fn physical_type(fields: Vec<(i32, PrimitiveType)>) -> StructType { + StructType::new( + fields + .into_iter() + .map(|(id, ty)| NestedField::optional(id, format!("physical_{id}"), ty.into()).into()) + .collect(), + ) +} + +fn tuple(values: Vec<Option<Literal>>) -> Struct { + values.into_iter().collect() +} + +fn assert_invalid<T: std::fmt::Debug>(result: crate::Result<T>, message: &str) { + let error = result.unwrap_err(); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert!(error.message().contains(message), "{error}"); +} + +#[test] +fn test_full_union_retains_dropped_sources() { + let old_schema = schema(vec![(1, PrimitiveType::String)]); + let current_schema = schema(vec![(2, PrimitiveType::Int)]); + let old = spec(0, vec![(1, 1000, "region", "identity")]); + let current = spec(1, vec![(2, 1001, "id", "identity")]); + let result = + StatisticsPartitioning::new(&[¤t, &old], &[¤t_schema, &old_schema], &[]) + .unwrap(); + assert_eq!( + result.partition_type(), + &StructType::new(vec![ + NestedField::optional(1000, "region", PrimitiveType::String.into()).into(), + NestedField::optional(1001, "id", PrimitiveType::Int.into()).into(), + ]) + ); + let active = crate::partitioning::compute_unified_partition_type( + [&old, ¤t].into_iter(), + ¤t_schema, + ) + .unwrap(); + assert_eq!(active.fields().len(), 1); + assert_eq!(active.fields()[0].id, 1001); +} + +#[test] +fn test_reordered_renamed_and_removed_fields_roundtrip() { + let sources = schema(vec![ + (1, PrimitiveType::String), + (2, PrimitiveType::Int), + (3, PrimitiveType::Long), + ]); + let old = spec(0, vec![ + (2, 1002, "id", "identity"), + (1, 1000, "region", "identity"), + ]); + let current = spec(1, vec![ + (1, 1000, "new_region", "identity"), + (3, 1005, "bucket", "bucket[8]"), + ]); + let resolver = StatisticsPartitioning::new(&[&old, ¤t], &[&sources], &[]).unwrap(); + let fields = resolver.partition_type().fields(); + assert_eq!( + fields + .iter() + .map(|field| (field.id, field.name.as_str(), field.required)) + .collect::<Vec<_>>(), + vec![ + (1000, "new_region", false), + (1002, "id", false), + (1005, "bucket", false) + ] + ); + let physical = physical_type(vec![ + (1000, PrimitiveType::String), + (1002, PrimitiveType::Int), + ]); + let key = resolver + .mapping(0, &physical) + .unwrap() + .key(&tuple(vec![ + Some(Literal::string("us")), + Some(Literal::int(7)), + ])) + .unwrap(); + assert_eq!(key.spec_id(), 0); + assert_eq!( + key.partition(), + &tuple(vec![Some(Literal::int(7)), Some(Literal::string("us"))]) + ); + let unified = resolver.unified_partition(&key).unwrap(); + assert_eq!( + unified, + tuple(vec![ + Some(Literal::string("us")), + Some(Literal::int(7)), + None + ]) + ); + assert_eq!( + resolver + .mapping(0, resolver.partition_type()) + .unwrap() + .key(&unified) + .unwrap(), + key + ); +} + +#[test] +fn test_manifest_evidence_recovers_a_dropped_source() { + let historical = spec(0, vec![(1, 1000, "dropped", "identity")]); + let physical = physical_type(vec![(1000, PrimitiveType::String)]); + let resolver = StatisticsPartitioning::new(&[&historical], &[], &[(0, &physical)]).unwrap(); + assert_eq!( + *resolver.partition_type().fields()[0].field_type, + PrimitiveType::String.into() + ); + assert_invalid( + StatisticsPartitioning::new(&[&historical], &[], &[]), + "Cannot resolve historical type", + ); + let unknown = schema(vec![(1, PrimitiveType::Unknown)]); + assert_invalid( + StatisticsPartitioning::new(&[&historical], &[&unknown], &[]), + "Cannot resolve historical type", + ); +} + +#[test] +fn test_fixed_result_transforms_need_no_source_evidence() { + for (transform, expected) in [ + ("bucket[8]", PrimitiveType::Int), + ("year", PrimitiveType::Int), + ("month", PrimitiveType::Int), + ("hour", PrimitiveType::Int), + ("day", PrimitiveType::Date), + ] { + let historical = spec(0, vec![(1, 1000, "dropped", transform)]); + let resolver = StatisticsPartitioning::new(&[&historical], &[], &[]).unwrap(); + assert_eq!( + *resolver.partition_type().fields()[0].field_type, + expected.into() + ); + } +} + +#[test] +fn test_promotions_canonicalize_keys_and_hashes() { + for (old_type, new_type, old_value, new_value) in [ + ( + PrimitiveType::Int, + PrimitiveType::Long, + Literal::int(i32::MAX), + Literal::long(i32::MAX), + ), + ( + PrimitiveType::Float, + PrimitiveType::Double, + Literal::float(1.5_f32), + Literal::double(1.5), + ), + ( + PrimitiveType::Decimal { + precision: 9, + scale: 2, + }, + PrimitiveType::Decimal { + precision: 18, + scale: 2, + }, + Literal::decimal(1234), + Literal::decimal(1234), + ), + ] { + let historical = spec(0, vec![(1, 1000, "value", "identity")]); + let old_schema = schema(vec![(1, old_type.clone())]); + let new_schema = schema(vec![(1, new_type.clone())]); + let old_physical = physical_type(vec![(1000, old_type)]); + let new_physical = physical_type(vec![(1000, new_type.clone())]); + for schemas in [[&old_schema, &new_schema], [&new_schema, &old_schema]] { + let resolver = StatisticsPartitioning::new(&[&historical], &schemas, &[ + (0, &old_physical), + (0, &new_physical), + ]) + .unwrap(); + assert_eq!( + *resolver.partition_type().fields()[0].field_type, + new_type.clone().into() + ); + let old_key = resolver + .mapping(0, &old_physical) + .unwrap() + .key(&tuple(vec![Some(old_value.clone())])) + .unwrap(); + let new_key = resolver + .mapping(0, &new_physical) + .unwrap() + .key(&tuple(vec![Some(new_value.clone())])) + .unwrap(); + assert_eq!(old_key, new_key); + assert_eq!(HashSet::from([old_key, new_key]).len(), 1); + } + } +} + +#[test] +fn test_physical_only_promotions_are_resolved() { + let historical = spec(0, vec![(1, 1000, "value", "truncate[10]")]); + let narrow = physical_type(vec![(1000, PrimitiveType::Int)]); + let wide = physical_type(vec![(1000, PrimitiveType::Long)]); + let resolver = + StatisticsPartitioning::new(&[&historical], &[], &[(0, &wide), (0, &narrow)]).unwrap(); + assert_eq!( + *resolver.partition_type().fields()[0].field_type, + PrimitiveType::Long.into() + ); + assert_invalid( + resolver.mapping(0, &physical_type(vec![(1000, PrimitiveType::Double)])), + "Cannot promote", + ); +} + +#[test] +fn test_conflicting_types_are_rejected() { + for (left, right) in [ + (PrimitiveType::Int, PrimitiveType::String), + (PrimitiveType::Int, PrimitiveType::Double), + (PrimitiveType::Timestamp, PrimitiveType::Timestamptz), + (PrimitiveType::Fixed(16), PrimitiveType::Uuid), + ( + PrimitiveType::Decimal { + precision: 9, + scale: 2, + }, + PrimitiveType::Decimal { + precision: 18, + scale: 3, + }, + ), + ] { + let historical = spec(0, vec![(1, 1000, "value", "identity")]); + let left_schema = schema(vec![(1, left)]); + let right_schema = schema(vec![(1, right)]); + assert_invalid( + StatisticsPartitioning::new(&[&historical], &[&left_schema, &right_schema], &[]), + "Conflicting types", + ); + } +} + +#[test] +fn test_source_conflicts_are_not_hidden_by_fixed_outputs() { + let historical = spec(0, vec![(1, 1000, "bucket", "bucket[8]")]); + let left_schema = schema(vec![(1, PrimitiveType::Int)]); + let right_schema = schema(vec![(1, PrimitiveType::String)]); + assert_invalid( + StatisticsPartitioning::new(&[&historical], &[&left_schema, &right_schema], &[]), + "Conflicting types", + ); +} + +#[test] +fn test_fixed_manifest_outputs_cannot_be_promoted() { + let historical = spec(0, vec![(1, 1000, "bucket", "bucket[8]")]); + let invalid = physical_type(vec![(1000, PrimitiveType::Long)]); + assert_invalid( + StatisticsPartitioning::new(&[&historical], &[], &[(0, &invalid)]), + "Invalid physical result type", + ); +} + +#[test] +fn test_v1_void_replacement_keeps_type_and_latest_name() { + let old = spec(0, vec![(1, 1000, "old_name", "identity")]); + let void = spec(1, vec![(1, 1000, "new_name", "void")]); + let old_physical = physical_type(vec![(1000, PrimitiveType::String)]); + let void_physical = physical_type(vec![(1000, PrimitiveType::Int)]); + let resolver = StatisticsPartitioning::new(&[&old, &void], &[], &[ + (0, &old_physical), + (1, &void_physical), + ]) + .unwrap(); + assert_eq!(resolver.partition_type().fields()[0].name, "new_name"); + assert_eq!( + *resolver.partition_type().fields()[0].field_type, + PrimitiveType::String.into() + ); + let mapping = resolver.mapping(1, &void_physical).unwrap(); + let key = mapping.key(&tuple(vec![None])).unwrap(); + assert_eq!(resolver.unified_partition(&key).unwrap(), tuple(vec![None])); + assert_invalid( + mapping.key(&tuple(vec![Some(Literal::int(1))])), + "Non-null value for void", + ); + let omitted = resolver + .mapping(1, &StructType::new(vec![])) + .unwrap() + .key(&Struct::empty()) + .unwrap(); + assert_eq!(omitted, key); +} + +#[test] +fn test_void_only_uses_available_type_or_int_fallback() { + let void = spec(0, vec![(1, 1000, "void", "void")]); + let sources = schema(vec![(1, PrimitiveType::String)]); + let physical = physical_type(vec![(1000, PrimitiveType::Long)]); + for (schemas, manifests, expected) in [ + (vec![&sources], vec![], PrimitiveType::String), + (vec![&sources], vec![(0, &physical)], PrimitiveType::String), + (vec![], vec![(0, &physical)], PrimitiveType::Long), + (vec![], vec![], PrimitiveType::Int), + ] { + let resolver = StatisticsPartitioning::new(&[&void], &schemas, &manifests).unwrap(); + assert_eq!( + *resolver.partition_type().fields()[0].field_type, + expected.into() + ); + } +} + +#[test] +fn test_conflicting_field_definitions_are_rejected_even_through_void() { + let old = spec(0, vec![(1, 1000, "value", "identity")]); + let different_source = spec(1, vec![(2, 1000, "value", "void")]); + assert_invalid( + StatisticsPartitioning::new(&[&old, &different_source], &[], &[]), + "Conflicting definitions", + ); + let conflicting = spec(1, vec![(1, 1000, "value", "truncate[8]")]); + let newest_void = spec(2, vec![(1, 1000, "value", "void")]); + assert_invalid( + StatisticsPartitioning::new(&[&newest_void, &old, &conflicting], &[], &[]), + "Conflicting definitions", + ); +} + +#[test] +fn test_unknown_and_zero_width_transforms_are_rejected() { + for transform in ["unknown", "bucket[0]", "truncate[0]"] { + let historical = spec(0, vec![(1, 1000, "value", transform)]); + assert_invalid( + StatisticsPartitioning::new(&[&historical], &[], &[]), + "Invalid transform", + ); + } +} + +#[test] +fn test_missing_necessary_field_differs_from_present_null() { + let sources = schema(vec![(1, PrimitiveType::String), (2, PrimitiveType::Int)]); + let old = spec(0, vec![(1, 1000, "region", "identity")]); + let current = spec(1, vec![(2, 1001, "id", "identity")]); + let resolver = StatisticsPartitioning::new(&[&old, ¤t], &[&sources], &[]).unwrap(); + let imported = physical_type(vec![(1001, PrimitiveType::Int)]); + assert_invalid( + resolver.mapping(0, &imported), + "Missing necessary partition field 1000", + ); + // Missing 1000 is irrelevant to spec 1; its physically present null is safe. + let key = resolver + .mapping(1, &imported) + .unwrap() + .key(&tuple(vec![None])) + .unwrap(); + assert_eq!(key.partition(), &tuple(vec![None])); + assert_eq!( + resolver.unified_partition(&key).unwrap(), + tuple(vec![None, None]) + ); +} + +#[test] +fn test_spec_id_preserves_distinct_keys() { + let sources = schema(vec![(1, PrimitiveType::Int)]); + let old = spec(0, vec![(1, 1000, "id", "identity")]); + let current = spec(1, vec![(1, 1000, "id", "identity")]); + let resolver = StatisticsPartitioning::new(&[&old, ¤t], &[&sources], &[]).unwrap(); + let values = tuple(vec![Some(Literal::int(5))]); + let old_key = resolver + .mapping(0, resolver.partition_type()) + .unwrap() + .key(&values) + .unwrap(); + let current_key = resolver + .mapping(1, resolver.partition_type()) + .unwrap() + .key(&values) + .unwrap(); + assert_ne!(old_key, current_key); + assert_eq!(HashSet::from([old_key, current_key]).len(), 2); +} + +#[test] +fn test_float_keys_preserve_signed_zero_and_normalize_nan() { + let sources = schema(vec![(1, PrimitiveType::Double)]); + let historical = spec(0, vec![(1, 1000, "value", "identity")]); + let resolver = StatisticsPartitioning::new(&[&historical], &[&sources], &[]).unwrap(); + let narrow = physical_type(vec![(1000, PrimitiveType::Float)]); + let mapping = resolver.mapping(0, &narrow).unwrap(); + let negative = mapping + .key(&tuple(vec![Some(Literal::float(-0.0_f32))])) + .unwrap(); + let positive = mapping + .key(&tuple(vec![Some(Literal::float(0.0_f32))])) + .unwrap(); + assert_ne!(negative, positive); Review Comment: My gut says both of these asserts should fail. `Literal::float` wraps `OrderedFloat`, and I believe `ordered-float` treats `-0.0 == 0.0` and hashes them the same. I haven't checked that against the pinned version, though. Can you confirm this test actually passes, and point me at which part of the code gives us the distinction? Keeping them distinct is the right behavior (Java's `Double.compare` does). But if it's `OrderedFloat` doing it and not `normalize`, I'd make the canonical form explicit in this module, for example by keying on `to_bits()`, rather than depending on a third-party `Eq`/`Hash` that a version bump could change. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
