Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate @ based syntax for accessing iceberg snapshots #10768

Merged
merged 2 commits into from
Jun 22, 2022

Conversation

phd3
Copy link
Member

@phd3 phd3 commented Jan 24, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

this deprecates @ based syntax for accessing iceberg snapshots.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

iceberg connector

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

() No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Iceberg
* Deprecate Iceberg snapshot access using mangled table names, in favor of the `AS OF` syntax. The old behavior can be restored by `allow_legacy_snapshot_syntax`  session property or `iceberg.allow-legacy-snapshot-syntax` config property. ({issue}`10768`)

@cla-bot cla-bot bot added the cla-signed label Jan 24, 2022
@phd3 phd3 force-pushed the iceberg-snapshot-disallow branch from c058bda to e32c0ad Compare January 24, 2022 18:24
@b-slim
Copy link
Contributor

b-slim commented Jan 25, 2022

Thanks, @phd3 for the patch I am wondering why shall we keep around this ambiguous access, I think it is better to fail when a snapshot id is not valid and the user can still access the latest snapshot when using table name with no snapshot. IOW this flag is yet another flag with no clear added value?

@phd3
Copy link
Member Author

phd3 commented Jan 25, 2022

I think it is better to fail when a snapshot id is not valid

Current behavior also allows interpreting that number as a TS. Since this syntax has been around for a while, IMO would be worth providing a switch before fully removing. but let's see what others think too

@phd3 phd3 requested review from findepi and electrum January 26, 2022 13:45
@phd3
Copy link
Member Author

phd3 commented Jan 26, 2022

Flaky #10752

@martint
Copy link
Member

martint commented Mar 3, 2022

AS OF syntax is already merged into Trino, BTW.

@electrum
Copy link
Member

Should we make this config an enum of BOTH, SNAPSHOT_ONLY, NONE?

@phd3 phd3 force-pushed the iceberg-snapshot-disallow branch 2 times, most recently from 442c95e to 443ec11 Compare June 7, 2022 13:13
@phd3
Copy link
Member Author

phd3 commented Jun 7, 2022

@electrum updated to add enums. would you like to take another look?

@findepi
Copy link
Member

findepi commented Jun 7, 2022

Once AS OF syntax support is merged, we can use the same conf to get rid of the syntax support altogether.

#10258 is merged now.

@phd3
Copy link
Member Author

phd3 commented Jun 7, 2022

@findepi @electrum is allow/deny sufficient now since #10258 merged? or still want to keep enum?

@findepi
Copy link
Member

findepi commented Jun 8, 2022

@phd3 i am fine with simple boolean flag

@phd3 phd3 force-pushed the iceberg-snapshot-disallow branch from 443ec11 to 7b3c30e Compare June 21, 2022 16:04
@phd3 phd3 force-pushed the iceberg-snapshot-disallow branch from 8d3507f to 5d4d517 Compare June 21, 2022 19:10
@phd3
Copy link
Member Author

phd3 commented Jun 21, 2022

@findepi ptal

@phd3 phd3 force-pushed the iceberg-snapshot-disallow branch from 5d4d517 to fa678d1 Compare June 22, 2022 14:40
@phd3 phd3 changed the title Disallow timestamp based snapshot access Deprecate @ based syntax for accessing iceberg snapshots Jun 22, 2022
@phd3 phd3 merged commit 288be0a into trinodb:master Jun 22, 2022
@github-actions github-actions bot added this to the 387 milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants