Skip to content

Use Iceberg Metadata table's API for snapshot metadata table#12776

Merged
electrum merged 1 commit intotrinodb:masterfrom
osscm:upgrade-snapshots-table-api
May 9, 2023
Merged

Use Iceberg Metadata table's API for snapshot metadata table#12776
electrum merged 1 commit intotrinodb:masterfrom
osscm:upgrade-snapshots-table-api

Conversation

@osscm
Copy link
Copy Markdown
Contributor

@osscm osscm commented Jun 9, 2022

Description

This will avoid using custom code to populate the Snapshot metadata table. This improvement will use the Iceberg's metadata table.scan, hence any changes to the underlying Iceberg's metadata table's scan logic will be used here. Spark engine also uses the Iceberg's metadata table.scan instead of custom code, so this will also help to reduce regression between Trino and Spark engines.

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

Improvement

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?

No change in the syntax or the usage of the $snapshots metadata table

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.

@cla-bot cla-bot bot added the cla-signed label Jun 9, 2022
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jun 10, 2022

CI failure relates to this change.

@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 10, 2022

cc @alexjo2144

@osscm
Copy link
Copy Markdown
Contributor Author

osscm commented Jun 10, 2022

CI failure relates to this change.

Error:  io.trino.plugin.iceberg.TestIcebergReadVersionedTable.testSelectTableWithEndShortTimestampWithTimezone  Time elapsed: 0.006 s  <<< FAILURE!
java.lang.AssertionError: 
For query: 
 SELECT * FROM test_iceberg_read_versioned_table FOR TIMESTAMP AS OF TIMESTAMP '+54409-04-16 13:40:21.000000000 Z'
not equal
Actual rows (up to 100 of 1 extra rows shown, 2 rows in total):
    [b, 2]
Expected rows (up to 100 of 0 missing rows shown, 1 rows in total):

yes, I see this query is failing, this seems to be not related to the $snapshot code change.
but let me check one more time.

Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

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

The test does look related, it reads from the Snapshots table here: https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergReadVersionedTable.java#L104

If you're sure though, just push an empty commit to trigger a rerun

@osscm osscm force-pushed the upgrade-snapshots-table-api branch 3 times, most recently from 4354935 to a271f6e Compare July 3, 2022 16:23
@osscm
Copy link
Copy Markdown
Contributor Author

osscm commented Jul 3, 2022

The test does look related, it reads from the Snapshots table here: https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergReadVersionedTable.java#L104

If you're sure though, just push an empty commit to trigger a rerun

@alexjo2144 there was an issue with the commit_at timestamp's long value length. Fixed it, and all test cases are green now. Can you please review when you get a chance. tests are cleared now, fault_tolerance tests are still red, checking that.

@osscm osscm force-pushed the upgrade-snapshots-table-api branch from a271f6e to 2cf559a Compare July 4, 2022 07:27
@findinpath findinpath self-requested a review July 5, 2022 19:03
@ebyhr ebyhr requested a review from alexjo2144 August 16, 2022 03:11
Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on the review. I'm okay with this change, pending the two small comments.

I'm just not sure if this adds much value unless we're deriving the table schema from the Iceberg SnapshotsTable#schema

@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Sep 26, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot cla-bot bot removed the cla-signed label Sep 26, 2022
@osscm osscm force-pushed the upgrade-snapshots-table-api branch from 30d170e to 6022663 Compare September 26, 2022 07:34
@cla-bot cla-bot bot added the cla-signed label Sep 26, 2022
@osscm osscm force-pushed the upgrade-snapshots-table-api branch 7 times, most recently from dcc6e01 to 0acce1f Compare September 26, 2022 23:58
@osscm osscm force-pushed the upgrade-snapshots-table-api branch 3 times, most recently from b61e331 to 6c21c36 Compare September 29, 2022 07:35
@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Feb 9, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@osscm osscm force-pushed the upgrade-snapshots-table-api branch from 060e196 to 5a90792 Compare February 9, 2023 05:14
@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Feb 9, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@osscm osscm force-pushed the upgrade-snapshots-table-api branch from 5a90792 to 96ae25c Compare February 9, 2023 05:24
@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Feb 9, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@osscm osscm force-pushed the upgrade-snapshots-table-api branch from 96ae25c to 575c1f7 Compare February 9, 2023 05:39
@cla-bot
Copy link
Copy Markdown

cla-bot bot commented Feb 9, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Manish Malhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@osscm osscm force-pushed the upgrade-snapshots-table-api branch from 575c1f7 to ed4476c Compare February 9, 2023 05:43
@cla-bot cla-bot bot added the cla-signed label Feb 9, 2023
@osscm osscm force-pushed the upgrade-snapshots-table-api branch from ed4476c to 0b7af26 Compare February 11, 2023 09:50
@osscm
Copy link
Copy Markdown
Contributor Author

osscm commented Feb 12, 2023

@ebyhr resolved the conflitcs

@ebyhr ebyhr self-requested a review February 12, 2023 12:04
@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Feb 17, 2023
Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Commit granularity is wrong. Please squash commits into one or separate them into appropriate commits.

@osscm osscm force-pushed the upgrade-snapshots-table-api branch 2 times, most recently from 4ee4dd3 to be3aa53 Compare March 17, 2023 23:51
@osscm
Copy link
Copy Markdown
Contributor Author

osscm commented Mar 17, 2023

Commit granularity is wrong. Please squash commits into one or separate them into appropriate commits.

thanks @ebyhr
Squashed the commits.

@github-actions github-actions bot added the iceberg Iceberg connector label Mar 18, 2023
@osscm osscm force-pushed the upgrade-snapshots-table-api branch from be3aa53 to 3fb2f77 Compare March 18, 2023 18:56
@osscm
Copy link
Copy Markdown
Contributor Author

osscm commented May 6, 2023

Commit granularity is wrong. Please squash commits into one or separate them into appropriate commits.

thanks @ebyhr Squashed the commits.

thanks for the review @ebyhr
Do you think we can push this PR now?

@electrum electrum merged commit 11ad368 into trinodb:master May 9, 2023
@github-actions github-actions bot added this to the 417 milestone May 9, 2023
@osscm
Copy link
Copy Markdown
Contributor Author

osscm commented May 13, 2023

Thanks a lot @ebyhr @electrum @colebow and @mosabua !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed iceberg Iceberg connector no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

Use Iceberg's Metadata table API's for the SnapshotsTable

6 participants