anuragmantri commented on code in PR #3128:
URL: https://github.com/apache/iceberg-rust/pull/3128#discussion_r3993948662
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -86,11 +88,19 @@ impl ManifestFileContext {
case_sensitive,
partition_spec,
unified_partition_type,
+ table_metadata,
} = self;
let manifest = object_cache.get_manifest(&manifest_file).await?;
for manifest_entry in manifest.entries() {
+ let sort_order = manifest_entry
+ .data_file()
+ .sort_order_id()
+ .and_then(|id| table_metadata.sort_order_by_id(id as i64))
Review Comment:
Added `FileScanTask::sort_order_id: Option<i32>` alongside `sort_order`,
carried through unresolved from `DataFile::sort_order_id()`. So now
`sort_order_id` distinguishes all three cases (absent, unresolvable, resolved)
while `sort_order` stays `None` unless the id resolves to a genuinely sorted
order. That should give the Part 2 optimizer what it needs even when the order
definition itself is gone.
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -86,11 +88,19 @@ impl ManifestFileContext {
case_sensitive,
partition_spec,
unified_partition_type,
+ table_metadata,
} = self;
let manifest = object_cache.get_manifest(&manifest_file).await?;
for manifest_entry in manifest.entries() {
+ let sort_order = manifest_entry
+ .data_file()
+ .sort_order_id()
+ .and_then(|id| table_metadata.sort_order_by_id(id as i64))
+ .filter(|order| !order.is_unsorted())
Review Comment:
Went with softening the doc rather than switching to `UNSORTED_ORDER_ID`,
since checking `is_unsorted()` on the resolved order is the behavior I actually
want (matches Java's `isSorted()` gate). Reworded so the doc leads with "an
order with no sort fields" and mentions id 0 only as the spec-defined example,
rather than implying the check keys off the id.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1640,6 +1640,76 @@ pub mod tests {
manifest_list_write.close().await.unwrap();
}
+ /// Writes a manifest with four live "Added" data-file entries
(partitioned on `x`
+ /// = 100, 200, 300, 400), each with the given `sort_order_id` set on
its `DataFile`
+ /// (`None` leaves the field unset). Used to test how `sort_order_id`
resolution
+ /// against the table's sort orders flows into each entry's
`FileScanTask`.
+ pub async fn setup_manifest_files_with_sort_order_ids(
+ &mut self,
+ sort_order_ids: [Option<i32>; 4],
+ ) {
+ let current_snapshot =
self.table.metadata().current_snapshot().unwrap();
+ let current_schema =
current_snapshot.schema(self.table.metadata()).unwrap();
+ let current_partition_spec =
self.table.metadata().default_partition_spec();
+ let parquet_file_size = self.write_parquet_data_files();
+
+ let mut writer = ManifestWriterBuilder::new(
+ self.next_manifest_file(),
+ Some(current_snapshot.snapshot_id()),
+ current_schema.clone(),
+ current_partition_spec.as_ref().clone(),
+ )
+ .build_v2_data();
+
+ for (i, sort_order_id) in sort_order_ids.into_iter().enumerate() {
+ let mut data_file_builder = DataFileBuilder::default();
+ data_file_builder
+ .partition_spec_id(0)
+ .content(DataContentType::Data)
+ .file_path(format!("{}/{}.parquet", &self.table_location,
i + 1))
+ .file_format(DataFileFormat::Parquet)
+ .file_size_in_bytes(parquet_file_size)
+ .record_count(1)
+ .partition(Struct::from_iter([Some(Literal::long(
+ 100 * (i as i64 + 1),
+ ))]));
+ if let Some(id) = sort_order_id {
+ data_file_builder.sort_order_id(id);
+ }
+ let data_file = data_file_builder.build().unwrap();
+
+ writer
+ .add_entry(
+ ManifestEntry::builder()
+ .status(ManifestStatus::Added)
+ .data_file(data_file)
+ .build(),
+ )
+ .unwrap();
+ }
+
+ let data_file_manifest =
writer.write_manifest_file().await.unwrap();
+
+ let manifest_list_writer = self
+ .table
+ .file_io()
+ .new_output(current_snapshot.manifest_list())
+ .unwrap()
+ .writer()
+ .await
+ .unwrap();
+ let mut manifest_list_write = ManifestListWriter::v2(
+ manifest_list_writer,
+ current_snapshot.snapshot_id(),
+ current_snapshot.parent_snapshot_id(),
+ current_snapshot.sequence_number(),
+ );
+ manifest_list_write
+ .add_manifests(vec![data_file_manifest].into_iter())
Review Comment:
Fixed for the helper this PR touches. Left the other manifest-list helpers
as `vec![...]` since they're pre-existing code outside this diff.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -1968,6 +2038,73 @@ pub mod tests {
}
}
+ #[tokio::test]
+ async fn test_plan_files_carries_sort_order_into_file_scan_task() {
+ let mut fixture = TableTestFixture::new();
+
+ let expected_sort_order = fixture
+ .table
+ .metadata()
+ .sort_order_by_id(3)
+ .unwrap()
+ .clone();
+
+ fixture
+ .setup_manifest_files_with_sort_order_ids([Some(3), None,
Some(99), Some(0)])
+ .await;
+
+ let tasks: Vec<_> = fixture
+ .table
+ .scan()
+ .build()
+ .unwrap()
+ .plan_files()
+ .await
+ .unwrap()
+ .try_collect()
+ .await
+ .unwrap();
+
+ assert_eq!(tasks.len(), 4, "expected all four FileScanTasks");
Review Comment:
Added two: an assertion that exactly one task resolves to
`sort_order.is_some()`, and one that three tasks carry a raw `sort_order_id`.
Both would catch the systemic-regression case you described.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -150,6 +150,22 @@ pub struct FileScanTask {
#[builder(default)]
pub unified_partition_type: Option<Arc<StructType>>,
+ /// The sort order that this file's rows are sorted by, resolved from the
data file's
+ /// `sort_order_id` against the table's known sort orders. `Some` only if
the id resolves
+ /// to a sort order with at least one field. `None` if the file has no
sort order id, the
+ /// id doesn't resolve against the table's sort orders, or it resolves to
the reserved
+ /// unsorted order (id 0, per the spec).
+ ///
+ /// Note: this reflects only the file's own recorded sort order id, not
necessarily the
+ /// table's current default sort order — see
[`crate::spec::DataFile::sort_order_id`].
Review Comment:
Fixed, now links to `sort_order_id()`. Also reran `cargo doc` with
`broken_intra_doc_links` denied to confirm.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -150,6 +150,22 @@ pub struct FileScanTask {
#[builder(default)]
pub unified_partition_type: Option<Arc<StructType>>,
+ /// The sort order that this file's rows are sorted by, resolved from the
data file's
+ /// `sort_order_id` against the table's known sort orders. `Some` only if
the id resolves
+ /// to a sort order with at least one field. `None` if the file has no
sort order id, the
+ /// id doesn't resolve against the table's sort orders, or it resolves to
the reserved
+ /// unsorted order (id 0, per the spec).
+ ///
+ /// Note: this reflects only the file's own recorded sort order id, not
necessarily the
+ /// table's current default sort order — see
[`crate::spec::DataFile::sort_order_id`].
+ /// Serde: not yet implemented.
+ #[serde(default)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ #[serde(serialize_with = "serialize_not_implemented")]
Review Comment:
Agreed, dropped the `not_implemented` guard. `SortOrder` already derives
`Serialize`/`Deserialize` and the workspace's serde already has the `rc`
feature on, so `Option<SortOrderRef>` round-trips with plain `#[serde(default,
skip_serializing_if = "Option::is_none")]`. Verified with a doc build too, no
wire-format reason to keep it stubbed.
##########
crates/iceberg/testdata/example_table_metadata_v2.json:
##########
@@ -41,6 +41,10 @@
"last-partition-id": 1000,
"default-sort-order-id": 3,
"sort-orders": [
+ {
+ "order-id": 0,
Review Comment:
Reverted the fixture change. The test now clones the table's metadata and
inserts the unsorted order inline (same pattern `new_unpartitioned` uses to
mutate a cloned `TableMetadata`), so the id-0 case exercises the real
`!is_unsorted()` filter instead of the fixture's absence letting `and_then`
short-circuit to the same result.
--
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]
