feat: add support for `_spec_id` metadata column by hsiang-c · Pull Request #2695 · apache/iceberg-rust

hsiang-c · 2026-06-22T23:34:17Z

Which issue does this PR close?

Partially closes Support for projecting metadata columns _pos, _spec_id, and _partition in table scan #2607

What changes are included in this PR?

If a projection includes _spec_id, which is a constant like the _file metadata column for all rows, add it to RecordBatchTransformerBuilder

Are these changes tested?

Yes, unit tests.
Iceberg Spark tests with Comet's 1.9.1.diff and temporarily turn off the check on metadata columns

Sample plan

== Physical Plan ==
AdaptiveSparkPlan isFinalPlan=false
+- CometSort [id#1247L, data#1248, _spec_id#1249], [id#1247L ASC NULLS FIRST]
   +- CometExchange rangepartitioning(id#1247L ASC NULLS FIRST, 200), ENSURE_REQUIREMENTS, CometNativeShuffle, [plan_id=3533]
      +- CometFilter [id#1247L, data#1248, _spec_id#1249], (id#1247L >= 10)
         +- CometIcebergNativeScan [id#1247L, data#1248, _spec_id#1249], /var/folders/d2/b93h6k7174ddqxltrxgb51040000gn/T/iceberg_warehouse16657344237870771906/iceberg_data/default/table/metadata/00004-70b616c5-1bbe-4e1d-bdd6-e9ac4910674a.metadata.json, testrest.default.table (branch=null) [filters=id IS NOT NULL, id >= 10, groupedBy=], 1

hsiang-c · 2026-06-22T23:35:15Z

FYI @advancedxy @parthchandra

advancedxy · 2026-06-23T05:41:15Z

+        if task
+            .project_field_ids()
+            .contains(&RESERVED_FIELD_ID_SPEC_ID)
+            && let Some(partition_spec) = &task.partition_spec


I think we should always add spec_id into the constant map if it's selected/projected.

The task.partition_spec should be present even for unpartitioned table(spec_id = 0).

+1. If the invariant is that task.partition_spec is always present (spec_id = 0 even for unpartitioned tables), then gating the column on let Some(partition_spec) means a projected _spec_id can silently produce no column when that invariant is ever violated, which would be a confusing 'column vanished' bug rather than a clean error. Adding _spec_id to the constant map whenever it's projected (per @advancedxy) is the safer contract. Worth a test on an unpartitioned table to lock in spec_id = 0.

@mbutrovich Thanks for the suggestion, I think it makes sense and I have a test_select_with_spec_id_column_from_unpartitioned_table for this case.

advancedxy · 2026-06-23T06:23:12Z

                partition_spec_id: manifest_file.partition_spec_id,
                bound_predicates: bound_predicates.clone(),
                snapshot_schema: snapshot_schema.clone(),
                delete_file_index: delete_file_index.clone(),
                name_mapping: name_mapping.clone(),
                case_sensitive,
+                partition_spec: partition_spec.clone(),


the partition spec is already in mainifest_file, like the partition_spec_id. maybe we could just use that and avoid embedding partition_spec into ManifestFileContext

advancedxy · 2026-06-23T06:24:40Z

@hsiang-c thanks, I think it's in good shape, just two minor comments.

mbutrovich

This is a clean, well-scoped PR. Reusing with_constant is exactly how Java treats _spec_id (a per-file constant in PartitionUtil.constantsMap), and the REE-vs-schema-type split in record_batch_transformer.rs looks like a genuine fix the other two PRs will want. Two questions before I'd feel good approving, both about test confidence rather than the implementation:

Both _spec_id tests assert 0 on single-spec fixtures. Could we add one where two files have different spec ids, since that's the case the column exists for?
The change edits the shared setup_manifest_files fixture (and its inspect/manifests.rs snapshot). Was that necessary, or could a dedicated fixture avoid weakening coverage for other tests?

(I've read @advancedxy's review. The "spec_id should always be added when projected / partition_spec is always present" point is already covered there, so I've turned mine into a +1 rather than a separate comment. The items below are the ones I didn't see raised yet.)

There is also a cross-PR shape question (below): I think the synthesized metadata-column mechanisms should converge, but the enum is better introduced in #2668 than here. This PR just needs to keep its scalar handling foldable.

Three sibling PRs each add a metadata column, and each currently reaches for a different mechanism: this PR (_spec_id) reuses the existing constant_fields: HashMap<i32, Datum> map; #2668 (_partition) adds partition_column: Option<PartitionColumnConstant>; #2746 (_pos) adds virtual_fields: HashSet<i32>.

Two of these are the same kind of thing and one isn't:

_file/_spec_id/identity partitions and _partition all synthesize a constant column (they already converge on ColumnSource::Add / AddStructConstant).
_pos is a different axis: arrow-rs RowNumber makes it a real column in the source batch, so it is passed through, not synthesized. It probably should stay its own small mechanism.

So the unification worth making is scalar-constant plus struct-constant (this PR's path plus #2668's), not all three. The natural place to introduce it is #2668, where the genuine second synthesis variant (StructConstant) arrives and both arms are test-covered at introduction (the existing _file test plus #2668's _partition tests). There's no good way to land that enum as a standalone PR first: the StructConstant arm has no producer or consumer until #2668, so it would be untested speculative code.

The only ask on this PR: keep the scalar-constant handling (the REE-vs-schema-type branch) in a shape that #2668 can fold into a MetadataColumnSource::ScalarConstant arm cleanly. Since this merges first in the suggested order (#2695 -> #2746 -> #2668), it sets the scalar baseline and #2668 generalizes on top. The full sketch is in the #2668 notes (a comment on its partition_column field; the author can move it to #2699 if that's a better home). cc the #2668 / #2746 authors.

mbutrovich · 2026-06-30T18:21:53Z

                                .file_size_in_bytes(parquet_file_size)
                                .record_count(1)
-                                .partition(Struct::from_iter([Some(Literal::long(100))]))
+                                .partition(Struct::from_iter([Some(Literal::long(1))]))


Collapsing three distinct partition values to 1/1/1 reduces distinctness for every test on setup_manifest_files and forces the inspect/manifests.rs bound edits below. Was there a failure with the original values that motivated this? If the new constant-type handling needs specific values, a separate fixture might keep the shared one intact. But if the old values actually broke, that itself sounds like a test worth keeping. What did you hit here?

Thanks @mbutrovich, good catch. It is actually a bug in my prior commits b/c I didn't distinguish field ids that are not present in a data file. This is now fixed.

mbutrovich · 2026-06-30T18:22:34Z

                StringArray
                [
-                  "100",
+                  "1",


This expected-output edit only exists because the fixture changed; if the fixture stays, this revert isn't needed. Flagging so reviewers know it's coupled to the above, not an independent behavior change.

This unnecessary change is reverted.

mbutrovich · 2026-06-30T18:22:51Z

        assert!(result.is_ok(), "Scan timed out - deadlock detected");
    }
+
+    #[tokio::test]


Both new tests assert 0 on a single-spec table. The whole point of _spec_id (https://iceberg.apache.org/spec/#reserved-field-ids) is distinguishing files written under different specs. A partition-evolved fixture where two files carry different spec ids would validate that directly. Would that be easy to add on top of the existing fixtures?

I added a new test fixture with evolved table partitions.

mbutrovich · 2026-06-30T18:23:02Z

+        if task
+            .project_field_ids()
+            .contains(&RESERVED_FIELD_ID_SPEC_ID)
+            && let Some(partition_spec) = &task.partition_spec


+1. If the invariant is that task.partition_spec is always present (spec_id = 0 even for unpartitioned tables), then gating the column on let Some(partition_spec) means a projected _spec_id can silently produce no column when that invariant is ever violated, which would be a confusing 'column vanished' bug rather than a clean error. Adding _spec_id to the constant map whenever it's projected (per @advancedxy) is the safer contract. Worth a test on an unpartitioned table to lock in spec_id = 0.

mbutrovich · 2026-06-30T18:24:22Z

+            && let Some(partition_spec) = &task.partition_spec
+        {
+            let spec_id_datum = Datum::int(partition_spec.spec_id());
+            record_batch_transformer_builder = record_batch_transformer_builder


Routing _spec_id through with_constant makes it RunEndEncoded (metadata-field constants get datum_to_arrow_type_with_ree), same as _file. The spec types _spec_id as a plain int (https://iceberg.apache.org/spec/#reserved-field-ids). For the Comet/Spark consumer, does REE get decoded cheaply downstream, or would a plain Int32 avoid a cast? Not necessarily a change, just confirming the consumer is happy with REE here as it is for _file.

mbutrovich · 2026-06-30T18:24:47Z

+                        datum_to_arrow_type_with_ree(datum)
+                    } else {
+                        field_id_to_mapped_schema_map
+                            .get(field_id)


Separating "metadata field becomes REE" from "identity-partition field becomes schema type" looks like a real correctness improvement: identity-partition constants shouldn't be forced to RunEndEncoded. Worth calling out to the #2668/#2746 authors so they rebase on top rather than reintroducing the old derivation. This branch is the scalar arm that #2668 would fold into a MetadataColumnSource::ScalarConstant; keeping it in this shape makes that easy.

Support selecting metadata column: _spec_id

0a25d45

hsiang-c force-pushed the meta_col_spec_id branch from 9279b1c to 0a25d45 Compare June 23, 2026 04:30

Fix CI

bcc3b6d

advancedxy reviewed Jun 23, 2026

View reviewed changes

mbutrovich mentioned this pull request Jun 30, 2026

feat: add support for _partition metadata column #2668

Open

mbutrovich self-requested a review June 30, 2026 18:06

mbutrovich reviewed Jun 30, 2026

View reviewed changes

hsiang-c added 2 commits July 1, 2026 23:03

style: inline w/ debug format

69e8714

Partition spec is always present

cf26ad1

hsiang-c force-pushed the meta_col_spec_id branch from 5ecd879 to bf527a6 Compare July 6, 2026 00:46

Test partition evolution

aee857e

hsiang-c force-pushed the meta_col_spec_id branch from bf527a6 to aee857e Compare July 6, 2026 00:48

hsiang-c and others added 7 commits July 6, 2026 11:28

Filter field ids which are not present in a data file

edaaf79

Fix typo checks

c8ea392

Merge branch 'main' into meta_col_spec_id

b9f8af5

Fix CI

24508e2

refactor: simplify control flow

5987a95

Make sure we read from Parquet, not partition values

bfd0977

Merge branch 'main' into meta_col_spec_id

84cc13c

Uh oh!

Conversation

hsiang-c commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

hsiang-c commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advancedxy commented Jun 23, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsiang-c Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hsiang-c commented Jun 22, 2026 •

edited

Loading

hsiang-c commented Jun 22, 2026 •

edited

Loading

mbutrovich Jun 30, 2026 •

edited

Loading

hsiang-c Jul 6, 2026 •

edited

Loading

mbutrovich Jun 30, 2026 •

edited

Loading