feat(rest): implement unregister table in catalogs

[mkroll-db]

Which issue does this PR close?

• Closes [RestCatalog] Implement unregister table in catalogs #3304.

What changes are included in this PR?

Adds catalog-level unregisterTable support introduced in #16400 in the REST catalog fixture and compatibility kit.

Unregister removes a table from the catalog without deleting its data or metadata files.
It returns a read-only table containing the last registered metadata. This can be used to register the table (with another catalog) again.

Are these changes tested?

Yes

AI Disclosure

Parts of the code were generated with AI.

[laskoviymishka]

Nice to see unregister land, and the REST side looks right — the endpoint and UnregisterTableResult match the spec's required metadata-location + metadata exactly, and the advertisement gating is handled well. Two things hold it back for me, though.

The SQL and memory backends delete the catalog entry before they read the metadata file that populates the return value. If that read fails on a transient blip — an S3 throttle, a bad cred, a missing file — the row is already gone and the caller never gets the metadata_location back, so there's no way to re-register. That's the opposite of the safety unregister is supposed to buy over drop_table. Both paths should read (and validate) first, then remove, mirroring what register_table already does in reverse; SQL's remove_table_at_location already returns a retryable conflict on 0 rows, so the swap is clean.

Second, unregister_table is a new required method on both Catalog and SessionCatalog with no default, so every downstream crate with its own impl stops compiling — and three of the six in-tree backends only return FeatureUnsupported anyway, which is the signal the baseline should be a default Err(FeatureUnsupported).

Before merge, I'd want:

• read-before-remove in the SQL and memory paths
• a default Err(FeatureUnsupported) on the trait method(s), so the stubs collapse into it and downstream impls keep building
• a test that asserts the metadata file still exists after unregister — today nothing proves files survive, which is the whole point of the feature

The rest is minor: Glue can implement this natively via delete_table, and I left a few small things inline. Once the ordering and the default land I'll take another pass.

[laskoviymishka]

Adding this as a required method with no default is a breaking change for every downstream crate implementing Catalog (and SessionCatalog in session.rs) — they all stop compiling. Since three of the six in-tree backends only return FeatureUnsupported, a default Err(FeatureUnsupported, "unregister_table is not supported by this catalog") is the natural baseline: the glue/hms/s3tables stubs collapse into it, and external impls keep building.

[laskoviymishka]

This removes the catalog entry before reading the metadata that populates the return value, so a transient read failure here leaves the table permanently unregistered with no recovery — the caller gets an error and never sees the metadata_location it would need to re-register. That's the opposite of the safety unregister is meant to buy over drop_table.

I'd read and validate the metadata first, then call remove_table_at_location. register_table already reads-before-inserts for exactly this reason, and the 0-rows conflict path stays intact:

let metadata = TableMetadata::read_from(&self.fileio, &metadata_location).await?;
self.remove_table_at_location(table_ident, &metadata_location).await?;

[laskoviymishka]

Same inversion as the SQL path: remove_existing_table drops the entry and releases the lock before read_from runs, so a failed read leaves the table irrecoverably unregistered. I'd get the metadata_location under the lock, read the metadata while still holding it (as load_table_from_locked_state does), then remove — the entry only disappears once the file is confirmed readable.

[laskoviymishka]

Every other TableNotFound in this file goes through no_such_table_err(table_ident), which formats with Debug — this one uses Display, so we end up with two different message strings for the same semantic error. Use the helper (it's already imported).

[laskoviymishka]

register_table guards non-V1 schemas with an early FeatureUnsupported; this path has no equivalent. If record_type_filter() behaves differently on V2, the SELECT can miss an existing row and return TableNotFound for a table that's actually there. I'd add the same guard, or add a V2 test that proves the round-trip.

[laskoviymishka]

Glue can do this natively — delete_table drops the catalog pointer without touching files, so unregister here is load_table + delete_table returning the read-only table, the same shape as the SQL path. Worth implementing rather than stubbing. For hms/s3tables the stub is fine, but I'd drop "yet" from the message so we're not implying a roadmap commitment we haven't made.

[laskoviymishka]

Callers of unregister_table only ever get a Table back — they never construct or inspect UnregisterTableResult. I'd make it pub(crate) so we're not permanently committing an internal HTTP-parsing DTO to the stable API. (CommitTableResponse being public is the existing precedent, but I'm not convinced that one's right either.)

[laskoviymishka]

The re-registration here proves the catalog entry can come back, but not that the metadata file was left untouched — which is the actual contract of unregister. I'd add a file_io.exists(metadata_location) assertion after the unregister (or a drop_table + read-back), so a future refactor that accidentally wires up file deletion gets caught.

[laskoviymishka]

This exercises the conflict path by calling the private remove_table_at_location directly, so the real end-to-end race (SELECT → concurrent update → CAS DELETE) never runs through unregister_table. Once the read-before-remove swap lands, I'd point this at the public unregister_table so it covers the actual sequence a caller hits.