New global state is breaking the Alter table scenarios

[ajantha-bhat]

After #2626 ,

main branch:
CREATE TABLE nessie.db1.T1 (col_a STRING, col_b STRING, col_c STRING) USING iceberg;
INSERT INTO nessie.db1.T1 VALUES ('foo', 'bar', 'baz');

create branch b1, b2 from main.
use branch b1

ALTER TABLE nessie.db1.T1 ADD COLUMN add_a STRING
INSERT INTO nessie.db1.T1 VALUES ('a', 'b', 'c', 'add')

use branch b2
ALTER TABLE nessie.db1.T1 ADD COLUMN add_b BIGINT   -- Iceberg throws validation exception with this error message "Cannot set invalid snapshot log: latest entry is not the current snapshot" 

Analysis:

Alter table operations goes as a simple transaction in Iceberg. While committing it, Iceberg cleans up snapshotLog in TableMetadata. During clean up it expects current snapshot id to be the head of snapshot log.
Refer code here: BaseTransaction::commitSimpleTransaction
this.current.removeSnapshotLogEntries(this.intermediateSnapshotIds)

But due to our global state change, now because of insert operation happened at b1, snapshot history head points to insert snapshot id. but b2's alter table operation expects snapshots head to be b2's snapshot id [Note: In iceberg, alter table operation doesn't generate new snapshot id. only schema id will be newly generated]

Solution
Keep a List<HistoryEntry> snapshotLog in on-reference-state of Iceberg, so that when b2's operation is in progress, it will just use b2's snapshot history.
Any other better solution ?

New solution:
Thinking more on this, I think instead of keeping snapshot history in Iceberg on-reference-state, we can compute it on the fly by querying nessie as we already have that information. This way each branch will use its own history and no Iceberg functionality will be broken including metadata tables.

When each time metadata refresh happens (we already have a hook to patch on-ref-state to table metadata pointer), we can query Nessie and collect the snapshot history from Nessie [by going through commits] and patch it up on TableMetadata.
Snapshot history needs snapshot timestamp. So, may be along with snapshot id, on-reference-state has to have snapshot timestamp. We cannot use commit timestamp there as some commits don't change snapshots also commit time is not exactly same as snapshot time, some issues in concurrent scenario may arise.

Also, for Iceberg's native snapshot/branching feature, if the Iceberg provides an interface to have snapshot history per reference. We should make use of it instead of changing our code.

[ajantha-bhat]

I also think we don't have end to end environment to test Iceberg + nessie due to spark extensions dependency on 0.9.2 Nessie client. At least for dev testing we should have it.

So, I will keep a draft PR for Nessie sql extensions which works on the latest client. So, developer can cherrypick this PR and build sql extensions and test end to end.

[rymurr]

I also think we don't have end to end environment to test Iceberg + nessie due to spark extensions dependency on 0.9.2 Nessie client. At least for dev testing we should have it.

Not that we shouldnt have an end to end test env but we can run iceberg + nessie integration tests w/o spark extensions. We can interact w/ nessie using cli or the java client directly.

IMHO we should have two integration tests:

1. head of iceberg/nessie
2. released of iceberg/nessie/extensions

[rymurr]

Solution
Keep a List snapshotLog in on-reference-state of Iceberg, so that when b2's operation is in progress, it will just use b2's snapshot history.
Any other better solution ?

This sounds potentially expensive. Is the recommendation to only store the last 1 entry? That sounds a bit better to me.

Is there no way to turn off this check or to turn off snapshot logging completely? This is a fix that has to be done as part of iceberg's branching anyways. What would the solution be if we changed iceberg instead of nessie?

[ajantha-bhat]

@rymurr

This sounds potentially expensive. Is the recommendation to only store the last 1 entry? That sounds a bit better to me.

I first thought this way only, but then snapshot history is always stored in chronological order. If I just store last entry per table@reference, clean up logic might go wrong as we change the ordering. So, I thought of storing per branch instead of global storage.

Is there no way to turn off this check or to turn off snapshot logging completely? This is a fix that has to be done as part of iceberg's branching anyways. What would the solution be if we changed iceberg instead of nessie?

I will check about this. I didn't find interface to turn it off yet.

[ajantha-bhat]

Not that we shouldnt have an end to end test env but we can run iceberg + nessie integration tests w/o spark extensions. We can interact w/ nessie using cli or the java client directly.

IMHO we should have two integration tests:

head of iceberg/nessie
released of iceberg/nessie/extensions

Agree. we can still test without spark extension. It is just for me that spark extension is more convenient.

[ajantha-bhat]

Thinking more on this, I think instead of keeping snapshot history in Iceberg on-reference-state, we can compute it on the fly by querying nessie as we already have that information. This way each branch will use its own history and no Iceberg functionality will be broken including metadata tables.

When each time metadata refresh happens (we already have a hook to patch on-ref-state to table metadata pointer), we can query Nessie and collect the snapshot history from Nessie [by going through commits] and patch it up on TableMetadata.
Snapshot history needs snapshot timestamp. So, may be along with snapshot id, on-reference-state has to have snapshot timestamp. We cannot use commit timestamp there as some commits don't change snapshots also commit time is not exactly same as snapshot time, some issues in concurrent scenario may arise.

Also, for Iceberg's native snapshot/branching feature, if the Iceberg provides an interface to have snapshot history per reference. We should make use of it instead of changing our code.

[rymurr]

It seems to me that we could just use the log functionality and take the 1st parent commit that has a different snapshot-id. The timestamps aren't important nor frankly is the accuracy. It just has to fix the above issue.

This is why we should work to fix it on the iceberg side. The snapshot log isnt a core part of the iceberg spec so it really shouldn't cause commits to hard fail imho

[ajantha-bhat]

@rymurr : After thinking some more time on this. I think Nessie's on-ref-state is having current snapshot id for the reference. From that we can still build the history snapshots as Snapshot object is having parentId.
So, no need to change on-ref-state or query Nessie log API or modify Iceberg-core code.

In our NessieTableOperations::validateRefreshedMetadata, we can form snapshots history using above information and patch the table metadata with that.

WDYT?

[rymurr]

I like your proposal. However I feel that addressing this at the iceberg level (based on the branchign documents) is better than patching nessie table metadata

[ajantha-bhat]

ok. Agree. It is concluded then.
I have added comment to snapshot branching PR (apache/iceberg#3104)

If it is not handled in that PR, I will work on it once #3104, #3425 is merged.

[snazy]

"Resetting" the snapshot-log in o.a.i.TableMetadata.withUpdatedSnapshotAndSchema() is the right thing IMO - at least until the Iceberg-snapshot-branching change is in. It's a rather simple change in the "bump Nessie to 0.14" PR in Iceberg.

[ajantha-bhat]

@snazy : The quick fix you did got reverted by this pr #3664

So, issue still exists.

I have raised this issue apache/iceberg#3850 to fix it properly long back. But no response yet.

cc: @rymurr

[snazy]

Does this issue still exist in current Nessie on main?

[ajantha-bhat]

As the global state is removed, this issue is not applicable anymore.