Skip to content

Verify accuracy of doing a CTAS statement from a versioned table#12839

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
findinpath:iceberg_ctas_versioned_table
Jun 22, 2022
Merged

Verify accuracy of doing a CTAS statement from a versioned table#12839
ebyhr merged 1 commit intotrinodb:masterfrom
findinpath:iceberg_ctas_versioned_table

Conversation

@findinpath
Copy link
Copy Markdown
Contributor

Description

Provide how-to test on performing cloning of a table at specific point in the past.

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

Test

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?

Provide how-to test on performing cloning of a table at specific point in the past.

Related issues, pull requests, and links

Documentation

( ) 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.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jun 14, 2022
@findinpath findinpath force-pushed the iceberg_ctas_versioned_table branch 2 times, most recently from e543415 to 23b23e3 Compare June 14, 2022 14:34
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment elsewhere

Suggested change
assertUpdate("INSERT INTO " + sourceTableName + " VALUES (1), (2), (3)", 3);
assertUpdate("INSERT INTO " + sourceTableName + " VALUES 1, 2, 3", 3);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Capitalization nit

Suggested change
private long getCommittedAtInEpochMilliSeconds(String tableName, long snapshotId)
private long getCommittedAtInEpochMilliseconds(String tableName, long snapshotId)

@findinpath findinpath force-pushed the iceberg_ctas_versioned_table branch from 23b23e3 to 09e514a Compare June 15, 2022 08:08
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
assertUpdate("CREATE TABLE " + sourceTableName + "(an_integer integer)");
assertUpdate("CREATE TABLE " + sourceTableName + "(an_integer INTEGER)");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

an_integer integer matches the convention in place used for the other tests.
Why do you suggest uppercasing the column type ?

@findinpath findinpath force-pushed the iceberg_ctas_versioned_table branch 2 times, most recently from bee1fca to 4814b5f Compare June 17, 2022 05:18
@findinpath findinpath requested a review from alexjo2144 June 17, 2022 05:19
Copy link
Copy Markdown
Member

@homar homar Jun 17, 2022

Choose a reason for hiding this comment

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

What is the point of this Thread.sleep? Next one makes sense but this one I don't understand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reasoning behind this Thread.sleep(1) is to avoid having two snapshots at the timestamp afterInsert123EpochMillis

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add a code comment.

@findinpath findinpath force-pushed the iceberg_ctas_versioned_table branch from 4814b5f to 6234469 Compare June 17, 2022 08:30
@findinpath findinpath requested a review from homar June 17, 2022 08:39
@homar
Copy link
Copy Markdown
Member

homar commented Jun 17, 2022

LGTM % maven checks

@findinpath findinpath force-pushed the iceberg_ctas_versioned_table branch from 6234469 to 1caecf5 Compare June 20, 2022 15:19
@findinpath findinpath force-pushed the iceberg_ctas_versioned_table branch from 1caecf5 to c524619 Compare June 22, 2022 01:44
@ebyhr ebyhr merged commit ff57246 into trinodb:master Jun 22, 2022
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jun 22, 2022

Merged, thanks!

@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label 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

Labels

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

Development

Successfully merging this pull request may close these issues.

5 participants