Skip to content

Remove redundant iceberg.compression-codec in product test#13706

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/iceberg-compression-product-test
Aug 18, 2022
Merged

Remove redundant iceberg.compression-codec in product test#13706
ebyhr merged 1 commit intotrinodb:masterfrom
ebyhr:ebi/iceberg-compression-product-test

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Aug 17, 2022

Description

Remove redundant iceberg.compression-codec in product test

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

ZSTD was unsupported for old Iceberg Avro tables, but
it's supported now.
@@ -1277,8 +1277,8 @@ public void verifyCompressionCodecsDataProvider()
assertThat(onTrino().executeQuery("SHOW SESSION LIKE 'iceberg.compression_codec'"))
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.

What is the purpose of this test ? Not sure why we want to assert on the default here.
Aren't we testing all compression codecs anyway ?

Copy link
Copy Markdown
Member Author

@ebyhr ebyhr Aug 18, 2022

Choose a reason for hiding this comment

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

What is the purpose of this test ?

I suppose the purpose is ensuring all supported compression codec in TestIcebergSparkCompatibility. If we add a new codec to the connector, this test will fail unless fixing storageFormatsAndCompressionCodecs

Aren't we testing all compression codecs anyway ?

We're testing all of them. Please take a look at TestIcebergSparkCompatibility.storageFormatsAndCompressionCodecs().

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.

Ah, okay got it now.
Could we update the assertion to match only the name and description columns ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wouldn't modify at least for now. We may want to catch the default session property change here.

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.

If the purpose is to ensure that all the supported compression codecs are tested, it doesn't look like we should care about change in the default compression codec here. But not a blocker for this PR.

@ebyhr ebyhr requested a review from raunaqmorarka August 18, 2022 06:10
@ebyhr ebyhr merged commit 41d9749 into trinodb:master Aug 18, 2022
@ebyhr ebyhr deleted the ebi/iceberg-compression-product-test branch August 18, 2022 07:34
@github-actions github-actions bot added this to the 394 milestone Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants