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

Update Iceberg from 1.3.1 to 1.4.1 #19434

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Oct 18, 2023

No description provided.

@cla-bot cla-bot bot added the cla-signed label Oct 18, 2023
@github-actions github-actions bot added the iceberg Iceberg connector label Oct 18, 2023
@findinpath findinpath requested review from ebyhr and findepi October 22, 2023 20:45
pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

LGTM % pom.xml changes

@ebyhr
Copy link
Member

ebyhr commented Oct 23, 2023

Please ping after Iceberg 1.4.1 is released officially.

@ebyhr ebyhr removed their request for review October 23, 2023 07:02
@findepi findepi marked this pull request as draft October 23, 2023 08:28
@@ -382,7 +382,7 @@ private static Metrics loadMetrics(TrinoInputFile file, HiveStorageFormat storag
InputFile inputFile = new ForwardingInputFile(file);
return switch (storageFormat) {
case ORC -> OrcMetrics.fromInputFile(inputFile, METRICS_CONFIG, nameMapping);
case PARQUET -> TrinoParquetUtil.fileMetrics(inputFile, METRICS_CONFIG, nameMapping);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@ebyhr ?

Copy link
Member

Choose a reason for hiding this comment

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

This change loos fine for me.

@nastra nastra marked this pull request as ready for review October 23, 2023 15:23
@@ -48,7 +48,6 @@ public static NessieIcebergClient createNessieIcebergClient(IcebergNessieCatalog
return new NessieIcebergClient(
HttpClientBuilder.builder()
Copy link
Member

Choose a reason for hiding this comment

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

HttpClientBuilder is deprecated now. Let's migrate to NessieClientBuilder in follow-up.

Copy link
Member

Choose a reason for hiding this comment

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

ACK

@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Oct 23, 2023
@ebyhr
Copy link
Member

ebyhr commented Oct 23, 2023

Please squash commits into one.

@ebyhr
Copy link
Member

ebyhr commented Oct 23, 2023

/test-with-secrets sha=1b4c5f8687c2b0e23efa8374def53cf4184340b0

@github-actions
Copy link

github-actions bot commented Oct 23, 2023

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/6620194009

@nastra nastra requested a review from ebyhr October 24, 2023 10:39
.withEnableApiCompatibilityCheck(false) was introduced as a workaround
and can be removed now. Details on the reasoning can be found at trinodb#17715 (comment)

Co-authored-by: Fokko Driesprong <[email protected]>
@ebyhr ebyhr merged commit bb66918 into trinodb:master Oct 24, 2023
88 of 89 checks passed
@mosabua
Copy link
Member

mosabua commented Oct 24, 2023

There are really no user facing improvements from this, and we therefore don't need a release notes entry @ebyhr @nastra @Fokko ?

@github-actions github-actions bot added this to the 431 milestone Oct 24, 2023
@nastra nastra deleted the iceberg-1.4.1 branch October 25, 2023 11:19
@@ -4704,7 +4704,7 @@ protected void verifyIcebergTableProperties(MaterializedResult actual)
assertThat(actual).isNotNull();
MaterializedResult expected = resultBuilder(getSession())
.row("write.format.default", format.name())
.build();
.row("write.parquet.compression-codec", "zstd").build();
Copy link
Contributor

Choose a reason for hiding this comment

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

@nastra this doesn't seem right - the parquet compression codec property is relevant only for parquet files (not for avro, orc).
#20401

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should adress this as part of apache/iceberg#9490

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.

8 participants