Skip to content

Support iceberg schema type change#223

Open
sfc-gh-abozkurt wants to merge 3 commits into
mainfrom
aykut/alter-iceberg-type
Open

Support iceberg schema type change#223
sfc-gh-abozkurt wants to merge 3 commits into
mainfrom
aykut/alter-iceberg-type

Conversation

@sfc-gh-abozkurt

@sfc-gh-abozkurt sfc-gh-abozkurt commented Feb 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

We should support schema type evolution according to iceberg spec v2, our current iceberg implementation. This should not require data file rewrite, but metadata-only operation.

  • make sure partition values of the existing files are interpreted correctly (the same value in old type) with the new type.
  • make sure data file stats of the existing files are interpreted correctly (the same value in old type) with the new type.
  • make sure to support all catalogs: postgres, rest and object_store.
  • add spark test
  • disallow it if it specifies "SET TYPE USING (expression)" syntax as this might require data file rewrite

Closes #222.

sfc-gh-abozkurt
sfc-gh-abozkurt commented Feb 23, 2026
View reviewed changes
Comment thread pg_lake_table/src/transaction/external_heavy_asserts.c
static void AssertInternalAndExternalIcebergDataFileColumnStatsMatch(List *internalColumnStatsList,
List *externalLowerBounds,
List *externalUpperBounds);
static void NormalizeColumnBound(Field * field, unsigned char *externalValue,

@sfc-gh-abozkurt sfc-gh-abozkurt Feb 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: we do not need these for the existing catalog values as we store them text. We can interpret them correctly when we cast their text value to new widen type.

That is only needed when we deserialize values of old type from parquet datafile or iceberg metadata.

@sfc-gh-abozkurt
sfc-gh-abozkurt force-pushed the aykut/alter-iceberg-type branch 2 times, most recently from 1c4ecae to 525f874 Compare February 27, 2026 16:26
sfc-gh-mslot
sfc-gh-mslot reviewed Mar 2, 2026
View reviewed changes
Comment thread pg_lake_table/src/ddl/alter_table.c
sfc-gh-mslot
sfc-gh-mslot reviewed Mar 2, 2026
View reviewed changes
Comment thread pg_lake_table/src/ddl/alter_table.c
@sfc-gh-abozkurt
sfc-gh-abozkurt force-pushed the aykut/alter-iceberg-type branch 5 times, most recently from a4c0a79 to d6610a0 Compare March 3, 2026 23:02
@sfc-gh-mslot

sfc-gh-mslot commented Mar 31, 2026

Copy link
Copy Markdown
Collaborator

trying to change the type to numeric(39,8) proceeds, but causes downstream failures:

SQL Compilation error: Invalid type modification for field 'B' from type '"decimal(32, 8)"'(nullability: 'true') to type '"string"'(nullability: 'true').

@sfc-gh-abozkurt
sfc-gh-abozkurt force-pushed the aykut/alter-iceberg-type branch from d6610a0 to da940ba Compare June 8, 2026 13:03
@sfc-gh-abozkurt

sfc-gh-abozkurt commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

trying to change the type to numeric(39,8) proceeds, but causes downstream failures:

SQL Compilation error: Invalid type modification for field 'B' from type '"decimal(32, 8)"'(nullability: 'true') to type '"string"'(nullability: 'true').

we should disallow when we widen above 38, which are not supported by iceberg (we fallback to text).

@sfc-gh-abozkurt @sfc-gh-mslot
Support iceberg schema type change
217ee36 
Signed-off-by: Aykut Bozkurt <aykut.bozkurt@snowflake.com>
@sfc-gh-abozkurt @sfc-gh-mslot
address
0fd9325
@sfc-gh-mslot
sfc-gh-mslot force-pushed the aykut/alter-iceberg-type branch from da940ba to 0fd9325 Compare June 23, 2026 08:59
@sfc-gh-mslot

sfc-gh-mslot commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Rebased onto main (conflict was just the import block in test_object_store_catalog.py — kept both sides). Build is clean and most tests pass. Three pg18 shards still fail though, all crash the postgres server, and they look like real regressions from this PR.

The pattern is partition values that are NULL. Test failures:

  • shard 3: test_string_edge_values — first insert is INSERT INTO t VALUES (null) into a partition_by='a' table.
  • shard 4: test_insert_select_null_partition_value — partitioned INSERT..SELECT with NULL rows in the partition column.
  • shard 5: test_vacuum_clamped_values[numeric-identity] — NaN clamped to NULL on a numeric partition column.

I think the trigger is in AssertInternalAndExternalIcebergStatsMatchForAllDataFiles. Before the PR, the per-partition-field comparison started with a length+memcmp on externalPartitionField->value. After the PR, it unconditionally calls NormalizePartitionFieldValue first, which then calls PGIcebergBinaryDeserialize(externalValue=NULL, externalLen=0, ...) for a NULL partition value. That's probably the segfault. Worth treating zero-length / NULL as identity in the normalize path (or short-circuiting before the call).

The numeric case may have a second issue too. NormalizePartitionFieldValue's switch lumps everything with physical_type == BINARY into BYTEAOID unless it's UUID. NUMERIC partition columns are stored as physical_type = BINARY with logical_type = DECIMAL (see partition_transform.c), so they fall through to bytea, which is wrong for the re-serialize step. Probably want a DECIMAL case that maps back to NUMERIC with the original typemod.

DCO still fails on this branch because the two commits don't have Signed-off-by — that one's just git commit --amend -s (or interactive rebase with -s) on your side.

Happy to take a look at the patch once you have a fix.

@sfc-gh-mslot @claude
Skip bound normalization for NULL partition values
366f81f 
The heavy-assert stats check now routes external partition values and
column bounds through PGIcebergBinaryDeserialize to widen them for type
promotions. That function asserts its input is non-NULL, so a NULL
partition value, such as a NULL row or a NaN numeric that Iceberg stores
as NULL, aborted assert-enabled backends during commit. This is why
test_string_edge_values, test_insert_select_null_partition_value, and
test_vacuum_clamped_values[numeric-identity] crashed the server in CI.

A NULL or empty value has no bytes to widen, so pass it through
unchanged. This matches the pre-promotion behaviour, where these values
compared equal with a zero-length memcmp.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Marco Slot <marco.slot@snowflake.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sfc-gh-mslot sfc-gh-mslot Awaiting requested review from sfc-gh-mslot

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support alter column set type for iceberg tables

2 participants

@sfc-gh-abozkurt @sfc-gh-mslot