Skip to content

Time travel engine improvements#12542

Merged
findepi merged 3 commits intotrinodb:masterfrom
findepi:findepi/time-travel-api
Jun 14, 2022
Merged

Time travel engine improvements#12542
findepi merged 3 commits intotrinodb:masterfrom
findepi:findepi/time-travel-api

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented May 25, 2022

No description provided.

@cla-bot cla-bot bot added the cla-signed label May 25, 2022
@findepi findepi mentioned this pull request May 25, 2022
Merged
findinpath
findinpath reviewed May 25, 2022
View reviewed changes
Comment thread core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated
findinpath
findinpath reviewed May 25, 2022
View reviewed changes
Comment thread core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated
jacob-stein
jacob-stein reviewed May 26, 2022
View reviewed changes
Comment thread core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated
@findepi findepi force-pushed the findepi/time-travel-api branch 2 times, most recently from 8c2ad46 to 2c8254b Compare June 7, 2022 10:36
@findepi findepi requested review from hashhar, losipiuk and martint and removed request for hashhar, losipiuk and martint June 7, 2022 10:37
@findinpath findinpath self-requested a review June 7, 2022 11:14
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 7, 2022

@losipiuk @martint mind taking a look ?

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 7, 2022

previously CI #12726

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 7, 2022

Rebasing after #10258 merged.

@findepi findepi force-pushed the findepi/time-travel-api branch from ec48889 to 1626d64 Compare June 7, 2022 20:12
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 8, 2022

CI #12726

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 8, 2022

Rebasing after #10258 merged.

adding small change to address #10258 (comment)
(i.e. this is why this PR got created)

@findepi findepi force-pushed the findepi/time-travel-api branch 2 times, most recently from e88535a to 50b12f2 Compare June 8, 2022 20:54
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 9, 2022

previous CI #12726

@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 9, 2022

conflict, rebasing

@findepi findepi force-pushed the findepi/time-travel-api branch 2 times, most recently from a4766ac to c026a60 Compare June 10, 2022 08:13
@findepi findepi force-pushed the findepi/time-travel-api branch 3 times, most recently from bffdacd to 23580da Compare June 10, 2022 08:37
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 10, 2022

Build is green again. There were some changes (#12542 (comment)). @losipiuk or @martint PTAL.

losipiuk
losipiuk reviewed Jun 13, 2022
View reviewed changes
Comment thread testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jun 13, 2022

Choose a reason for hiding this comment

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

I am not sure I get the intent here. Is there expectation from specific connector to provide more verbose version of this test which actually verifies specifically which query should fail and which should not?

Copy link
Copy Markdown
Member Author

@findepi findepi Jun 14, 2022

Choose a reason for hiding this comment

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

My expectation is that a connector supporting time travel will have its own test covering it.
Here, we only cover cases and verify they don't end up with some NPE, IAE, ISE or something.

losipiuk
losipiuk approved these changes Jun 13, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Looks nice

findinpath
findinpath reviewed Jun 13, 2022
View reviewed changes
Comment thread core/trino-spi/src/test/java/io/trino/spi/TestSpiBackwardCompatibility.java Outdated
findepi added 3 commits June 14, 2022 10:54
@findepi
Group ConnectorMetadata.getTableHandle overloads together
54719d4 
Move versioned `getTableHandle` under the plain one.
@findepi
Report Not Found also for versioned queries
625fc85 
When querying a table `FOR VERSION` there are these potential failure
scenarios

1. The table doesn't exist.
2. The connector doesn't support versioning at all.
3. The connector does not support given version type is not supported
   (e.g. supported versions are `bigint` (as in Iceberg) but user.
  provided a `varchar`)
4. The version type is fine, but requested version doesn't exist (e.g.
   bad Iceberg snapshot id).

Previously, when connector doesn't support versioning *and* table
doesn't exist, the message would indicate unsupported versioning, while
informing about table not being found is more appropriate.

To further differentiate between failure scenarios 2, 3 and 4, the
`ConnectorMetadata.isValidTableVersion` is removed. Instead, we rely on
`ConnectorMetadata.getTableHandle` to do proper checks (is versioning
supported? is version type supported? is version valid?). A sane
implementation will do such checks anyway, so this simplifies the SPI.
@findepi
Unify ConnectorMetadata.getTableHandle invocations
6b62b43 
If a connector implements table versioning, it's going to implement
`getTableHandle` that accepts optional version specification. It
shouldn't need to still implement the `getTableHandle` without
versioning specification.
@findepi findepi force-pushed the findepi/time-travel-api branch from 23580da to 6b62b43 Compare June 14, 2022 08:55
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 14, 2022

(rebased to resolve conflict)

@findepi findepi merged commit 2a466ed into trinodb:master Jun 14, 2022
@github-actions github-actions bot added this to the 386 milestone Jun 14, 2022
@colebow colebow mentioned this pull request Jun 14, 2022
Merged
@colebow
Copy link
Copy Markdown
Member

colebow commented Jun 14, 2022

Do we want release notes for this?

@findepi findepi deleted the findepi/time-travel-api branch June 15, 2022 09:58
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Jun 15, 2022
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jun 15, 2022

Do we want release notes for this?

@colebow
I didn't think we need, but @homar reported this accidentally fixed history + table redirections.

@homar can you please suggest release note wording and also add a test?

@homar
Copy link
Copy Markdown
Member

homar commented Jun 15, 2022

Test is added in scope of #12831
Regarding release not something like:
Fixes time travel for Iceberg's tables redirected through hive.
Without this AS OF syntax didn't work for table with redirection

@findepi findepi removed the no-release-notes This pull request does not require release notes entry label Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@losipiuk losipiuk losipiuk approved these changes

@rajarshisarkar rajarshisarkar Awaiting requested review from rajarshisarkar

@hashhar hashhar Awaiting requested review from hashhar

@martint martint Awaiting requested review from martint

@jacob-stein jacob-stein Awaiting requested review from jacob-stein

+1 more reviewer

@findinpath findinpath findinpath approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Milestone

386

Development

Successfully merging this pull request may close these issues.

7 participants

@findepi @colebow @homar @martint @losipiuk @jacob-stein @findinpath