pg_lake_iceberg: one-sided primitives for the stored Iceberg representation of a type by sfc-gh-okalaci · Pull Request #495 · Snowflake-Labs/pg_lake

sfc-gh-okalaci · 2026-07-30T09:27:31Z

An alternative shape for #488. Same base commit, so the diffs compare directly.

The derivation rework in #488 is right. This PR differs only in what gets exported.

One code path, exercised by every caller

register_field_ids.c:247-254 is already PostgresTypeToIcebergField + DeepCopyField + ApplyCompatibilityStorageMapping — exactly the shaping a consumer needs to reproduce. This extracts it into IcebergStorageFieldForColumnType and has registration call it, so there is one implementation rather than a second copy beside the original.

That matters more than DRY usually does here, because this whole area is edge cases. A duplicated model is only as correct as the tests that pin it, and for nested oversized numerics, nested uuid under compat, or interval-as-struct, those tests don't get written — which is how the top-level-only bug survived in the first place. With one path, every CREATE TABLE in the suite drives the same function, so a divergence surfaces as a broken create-path test instead of a wrong true in a guard nobody is watching.

The numeric rule is likewise not re-implemented. ConvertTypeTree is, by its own comment, "the single place that knows how pg_lake types nest ... so independent passes cannot drift out of coverage"; IcebergStoredPostgresType wraps the existing MaybeConvertType, and TypeHasUnsupportedNumericLeaf reuses that same traversal with a leaf rule that records instead of rewriting (so it walks the structure the real rewrite would and materializes nothing).

Only the new side should be derived

Deriving both sides answers the weaker "would a fresh create of old and new agree under today's settings", and fails in the unsafe direction: a transform not modelled here cancels out on both sides and yields a spurious match. Against a persisted field from field_id_mappings, the same gap makes them differ, so the caller blocks.

This is load-bearing because the transform set is not closed — snowflake_cdc stores enums and user-defined ranges as text before the table is created, outside pg_lake entirely. Only the persisted field reflects all of the transforms. The header says so.

Consequently there is no IcebergCreatePathContext: compatibility mode comes from the relation, and the GUC is read where pg_lake already reads it. The historical GUC value is already baked into the persisted field, so there is nothing to capture.

What this avoids

No leaf-conversion callback, no NULL Field, no PostgresTypeToIcebergFieldInternal split. iceberg_field.c is untouched and its 11 call sites keep a single postcondition. Note the other two storage transforms are already post-derivation Field-tree passes (ApplyCompatibilityStorageMapping, CollectStorageDivergences), and the read/write codecs already walk surface and storage trees in parallel — threading a callback into the derivation is the odd one out.

Roughly 23% less production code than #488.

Tests

pg_lake_iceberg/tests/pytests/test_iceberg_representation.py carries over #488's type-pair cases essentially unchanged (the GUC is pinned with SET rather than a 4th wrapper argument), and adds a layer that pins the exact stored type per Postgres type — numeric(50,2)[] is list<double>, not list<string>. Pairwise same/different verdicts can agree for the wrong reason when a derivation is wrong identically on both sides, which is the original bug.

The extraction itself is covered end to end by the existing create-path suites (pg_lake_table test_compatibility_mode.py, test_iceberg_uuid_compat.py).

CI green: 63/63 jobs.

The consumer side is the alternative to snowflake-eng/sfpg-extension-pg_lake_replication#704, one commit on top of that branch.

🤖 Generated with Claude Code

snowflake-security-bot

Snowflake Security Review

Security grade: A — Passed ✅

No security findings after adjudication. This PR passes the Snowflake Security Review.

📊 3 of 5 files (2 tests skipped) · 789 lines reviewed · 5 candidates → 1 kept · retrieval: on

sfc-gh-dachristensen · 2026-07-30T14:12:19Z

+ *
+ * That matters because the transform set is not closed.  pg_lake shapes storage
+ * with the unsupported-numeric rewrite and the compatibility mapping, but a
+ * caller may add its own before the table is created (snowflake_cdc stores enums


Always remember don't reference upstream projects in pg_lake source code, unless they are part of pg_lake proper.

Done — removed the snowflake_cdc example, kept the sentence generic: "a caller may add its own before the table is created."

sfc-gh-dachristensen · 2026-07-30T15:54:07Z

+ * Rewriting a nested leaf means rebuilding the enclosing composite or map, which
+ * materializes a type in lake_struct / the map schema -- the create path's own
+ * behaviour.  A caller that must not create catalog types should first reject
+ * the input with TypeHasUnsupportedNumericLeaf(type, true).


TypeHasUnsupportedNumericLeaf

Wonder if a more generic name might be useful for this check; while that might be the only case we currently have, there could be other situations that end up; maybe something about "RepresentationCreatesType" or some indication as to the side effects that we actually care about—that way callers won't all need to modify code if we add new conditions.

Renamed to TypeHasUnrepresentableLeaf. Drops the numeric-specific framing; the new name covers any leaf the Iceberg storage path cannot represent natively without conversion, regardless of what future conditions may be added.

sfc-gh-dachristensen · 2026-07-30T15:56:11Z

+ * Shares ConvertTypeTree's traversal, so it cannot drift out of coverage from
+ * the rewrite it predicts, and creates no catalog types (see the leaf rule).
+ */
+extern PGDLLEXPORT bool TypeHasUnsupportedNumericLeaf(PGType type, bool nestedOnly);


See my above comment on the naming/framing of this function.

Addressed in the same commit: TypeHasUnsupportedNumericLeaf → TypeHasUnrepresentableLeaf, and IcebergFieldsEquivalent moved to test-only.

sfc-gh-dachristensen · 2026-07-30T15:57:24Z

+
+
+/*
+ * IcebergFieldsEquivalent - see iceberg_representation.h.


nit: do we need to refer to a file's own header? seems pretty obvious.

Agreed, dropped all three. The function bodies stand on their own.

snowflake-security-bot

Snowflake Security Review

Security grade: A — Passed ✅

No security findings after adjudication. This PR passes the Snowflake Security Review.

📊 3 of 5 files (2 tests skipped) · 723 lines reviewed · retrieval: on

snowflake-security-bot

Snowflake Security Review

Security grade: A — Passed ✅

No security findings after adjudication. This PR passes the Snowflake Security Review.

📊 3 of 5 files (2 tests skipped) · 779 lines reviewed · retrieval: on

…a type A consumer deciding whether a type change is invisible to an existing Iceberg table (ALTER COLUMN ... TYPE) needs to know what the create path stores for the new type, so it can compare that against what the table already persists in lake_table.field_id_mappings. Export three primitives for that, in iceberg_representation.{c,h}: IcebergStoredPostgresType - the declared type after the unsupported numeric -> float8 rewrite, gated on the same GUC the create path gates it on IcebergStorageFieldForColumnType - the derivation plus the compatibility storage mapping IcebergFieldsEquivalent - structural equality of two field trees, ignoring per-derivation field ids and defaults TypeHasUnsupportedNumericLeaf - probe for a numeric leaf Iceberg cannot hold as a decimal, so a caller that must not create catalog types can fail closed The shaping is not a model of the create path, it IS the create path: CreatePostgresColumnMappingsForColumnDefs now calls IcebergStorageFieldForColumnType instead of open-coding PostgresTypeToIcebergField + DeepCopyField + ApplyCompatibilityStorageMapping, so a comparison against a persisted field runs the code that produced it and the two cannot drift. The numeric rewrite is likewise not re-implemented: IcebergStoredPostgresType wraps the existing MaybeConvertType, which already recurses through arrays, composites, maps and domains via ConvertTypeTree. TypeHasUnsupportedNumericLeaf reuses that same traversal with a leaf rule that records instead of rewriting, so it walks the structure the real rewrite would and materializes nothing. Only the new type should be derived. Deriving both sides answers the weaker "would a fresh create of old and new agree under today's settings", and fails in the unsafe direction: a transform not modelled here cancels out on both sides and yields a spurious match. Against a persisted field the same gap makes them differ, so the caller blocks. This matters because the transform set is not closed -- snowflake_cdc, for one, stores enums and user-defined ranges as text before the table is created. The header says so. Tests (pg_lake_iceberg/tests/pytests/test_iceberg_representation.py) pin the exact stored type per Postgres type -- numeric(50,2)[] is list<double>, not list<string> -- rather than only pairwise same/different verdicts, which can agree for the wrong reason when a derivation is wrong the same way on both sides. Type-pair expectations, both GUC states and the depth-dependent snowflake uuid mapping are covered on top of that. The existing create-path suites (pg_lake_table test_compatibility_mode.py, test_iceberg_uuid_compat.py) cover the extraction end to end. Signed-off-by: Onder KALACI <oender.kalaci@snowflake.com>

IcebergFieldsEquivalent has no callers in the pg_lake production tree; its only use is the pg_lake_same_iceberg_representation test wrapper. Move it to test_iceberg_representation.c as a static. Consumers outside pg_lake (snowflake_cdc) hold their own copy -- the function is self-contained and has no catalog side effects. TypeHasUnsupportedNumericLeaf -> TypeHasUnrepresentableLeaf. The name was too specific to the current single condition (oversized numerics); the new name is forward-compatible if other unrepresentable leaf types are added, and mirrors the established TypeHasStorageDivergentLeaf naming pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Onder KALACI <oender.kalaci@snowflake.com>

…plate Remove the snowflake_cdc example from iceberg_representation.h: pg_lake source should not reference upstream projects. The point is generic -- "a caller may add its own transform" -- and doesn't need an example. Drop the "FunctionName - see iceberg_representation.h" prefix from all three function bodies in iceberg_representation.c: the file already includes its own header, and cross-referencing it in every comment is noise. Also fix the probe struct comment that still named the old TypeHasUnsupportedNumericLeaf. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Onder KALACI <oender.kalaci@snowflake.com>

The previous commit moved it to test-only on the assumption it had no production callers within pg_lake. snowflake_cdc#704 (ALTER COLUMN TYPE noop detection) calls it directly, so it belongs in the public API. Restore to iceberg_representation.{h,c}; remove the temporary static copy from test_iceberg_representation.c, which now calls the exported symbol via the header it already includes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Onder KALACI <oender.kalaci@snowflake.com>

Whitespace-only: declaration alignment, Field * spacing and continuation indentation for the new iceberg_representation primitives and their test wrapper. No functional change. Signed-off-by: Onder KALACI <oender.kalaci@snowflake.com>

snowflake-security-bot

Snowflake Security Review

Security grade: A — Passed ✅

No security findings after adjudication. This PR passes the Snowflake Security Review.

📊 3 of 5 files (2 tests skipped) · 779 lines reviewed · retrieval: on

sfc-gh-okalaci requested review from sfc-gh-abozkurt, sfc-gh-amzheng, sfc-gh-dachristensen, sfc-gh-mslot and sfc-gh-npuka as code owners July 30, 2026 09:27

sfc-gh-okalaci mentioned this pull request Jul 30, 2026

pg_lake_iceberg: add SameIcebergStoredRepresentation helper #488

Closed

snowflake-security-bot Bot reviewed Jul 30, 2026

View reviewed changes

sfc-gh-dachristensen reviewed Jul 31, 2026

View reviewed changes

snowflake-security-bot Bot reviewed Aug 3, 2026

View reviewed changes

sfc-gh-dachristensen approved these changes Aug 3, 2026

View reviewed changes

sfc-gh-okalaci and others added 5 commits August 3, 2026 18:56

pg_lake_iceberg: apply pgindent formatting

d57d4ec

Whitespace-only: declaration alignment, Field * spacing and continuation indentation for the new iceberg_representation primitives and their test wrapper. No functional change. Signed-off-by: Onder KALACI <oender.kalaci@snowflake.com>

sfc-gh-okalaci force-pushed the okalaci/iceberg-stored-field-primitive branch from 9c30e40 to d57d4ec Compare August 3, 2026 15:58

snowflake-security-bot Bot reviewed Aug 3, 2026

View reviewed changes

sfc-gh-okalaci merged commit ec89ae7 into main Aug 3, 2026
64 checks passed

sfc-gh-okalaci deleted the okalaci/iceberg-stored-field-primitive branch August 3, 2026 16:19

Navigation Menu

Uh oh!

pg_lake_iceberg: one-sided primitives for the stored Iceberg representation of a type - #495

pg_lake_iceberg: one-sided primitives for the stored Iceberg representation of a type#495
sfc-gh-okalaci merged 5 commits into
mainfrom
okalaci/iceberg-stored-field-primitive

sfc-gh-okalaci commented Jul 30, 2026

Uh oh!

snowflake-security-bot Bot left a comment

Uh oh!

sfc-gh-dachristensen Jul 30, 2026

Uh oh!

sfc-gh-okalaci Aug 3, 2026

Uh oh!

sfc-gh-dachristensen Jul 30, 2026

Uh oh!

sfc-gh-okalaci Aug 3, 2026

Uh oh!

Uh oh!

sfc-gh-dachristensen Jul 30, 2026

Uh oh!

sfc-gh-okalaci Aug 3, 2026

Uh oh!

sfc-gh-dachristensen Jul 30, 2026

Uh oh!

sfc-gh-okalaci Aug 3, 2026

Uh oh!

snowflake-security-bot Bot left a comment

Uh oh!

snowflake-security-bot Bot left a comment

Uh oh!

snowflake-security-bot Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sfc-gh-okalaci commented Jul 30, 2026

One code path, exercised by every caller

Only the new side should be derived

What this avoids

Tests

Uh oh!

snowflake-security-bot Bot left a comment

Choose a reason for hiding this comment

Snowflake Security Review

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!

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!

snowflake-security-bot Bot left a comment

Choose a reason for hiding this comment

Snowflake Security Review

Uh oh!

snowflake-security-bot Bot left a comment

Choose a reason for hiding this comment

Snowflake Security Review

Uh oh!

snowflake-security-bot Bot left a comment

Choose a reason for hiding this comment

Snowflake Security Review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants