-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix failure reading checkpoint created by CREATE OR REPLACE with different schema
#27886
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
base: master
Are you sure you want to change the base?
Fix failure reading checkpoint created by CREATE OR REPLACE with different schema
#27886
Conversation
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
Outdated
Show resolved
Hide resolved
This is a bug fix to the write logic, right? The current release note entry looks misleading if so. |
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void testCreateReplaceReadingCheckpointWithDifferentSchema() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes even without this PR change. I think we should add SELECT statement to L295-296.
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java
Outdated
Show resolved
Hide resolved
Praveen2112
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does spark or other engines or delta lake spec mentions about writing checkpoint after we run CREATE OR REPLACE operation ?
I tried on Databricks (Spark OSS does not support s3 listing FWIW on Databricks, if the schema of the table is not being changed, there is no new checkpoint being created. |
|
|
||
| if (replaceExistingTable) { | ||
| writeCheckpointIfNeeded(session, schemaTableName, location, tableHandle.toCredentialsHandle(), tableHandle.getReadVersion(), checkpointInterval, commitVersion); | ||
| writeCheckpoint(session, schemaTableName, location, tableHandle.toCredentialsHandle(), commitVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pointed out in #27886 (comment) if the schema of the table does not change, there is no new checkpoint created by default (if it doesn't correspond to the checkpoint interval property) in DBX. Consider whether it makes sense to follow the same pattern here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - We would also think about handling about schema evolution (we don't support it now) - but when we support it do we need to create a new checkpoint for each alter operation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just tested dbx, only when changing column type it will add a checkpoint.
…with different schema
e6cd65c to
ce0932d
Compare
Description
Close #27805
write a checkpoint for
CREATE OR REPLACEAdditional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: