Core: Default to zstd compression for Parquet in new tables#8593
Core: Default to zstd compression for Parquet in new tables#8593rdblue merged 1 commit intoapache:masterfrom
Conversation
| Assert.assertEquals(Collections.singletonMap("dummy", "test"), table.properties()); | ||
| Map<String, String> expectedProperties = defaultProperties(); | ||
| expectedProperties.put("dummy", "test"); | ||
| Assert.assertEquals(expectedProperties, table.properties()); |
There was a problem hiding this comment.
I think it's fine to have tests for specific catalogs, but I don't think that setting this property is a general behavior of all catalogs that we want to validate. I'd remove these tests because this is up to the catalog implementation and it is fine if a catalog implementation doesn't apply this default. It can still be a valid implementation.
There was a problem hiding this comment.
Are you suggesting to modify all tests to ensure dummy key is present instead of validating the actual set of properties?
There was a problem hiding this comment.
Yes. Tests like these should ensure that the property passed in is present in properties, not that we have the exact set of properties.
There was a problem hiding this comment.
I think the tests should assert that the expected properties are a subset of actual properties and should not expect or assert that there are any specific default properties added. That's the basic contract here.
There was a problem hiding this comment.
Makes sense. Updated tests.
aokolnychyi
force-pushed
the
zstd-default
branch
2 times, most recently
from
22f9039 to
a10d3c5
Compare
Co-authored-by: Szehon Ho <szehon_ho@apple.com> Co-authored-by: Kyle Bendickson <kjbendickson@gmail.com>
aokolnychyi
force-pushed
the
zstd-default
branch
from
a10d3c5 to
db67144
Compare
|
Thanks, @szehon-ho, @aokolnychyi, and @kbendick! |
|
This introduced a new property to the table: https://github.com/trinodb/trino/pull/19188/files#diff-f5cd96d1ec33fae497df9613afcefff034f01c99adf961e927ccd76e637c7b81R4704-R4711 |
|
The |
4 participants
This PR is an updated and rebased version of #8299.