Fix table props for Iceberg with orc_bloom_filter #20433
Conversation
oneonestar
force-pushed
the
fix.orc.bloom.filter
branch
from
387bc46 to
24bb55b
Compare
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java
Outdated
Show resolved
Hide resolved
| @@ -662,8 +687,8 @@ public static Map<String, String> createTableProperties(ConnectorTableMetadata t | |||
| if (!columns.isEmpty()) { | |||
| checkFormatForProperty(fileFormat.toIceberg(), FileFormat.ORC, ORC_BLOOM_FILTER_COLUMNS); | |||
| validateOrcBloomFilterColumns(tableMetadata, columns); | |||
| propertiesBuilder.put(ORC_BLOOM_FILTER_COLUMNS_KEY, Joiner.on(",").join(columns)); | |||
| propertiesBuilder.put(ORC_BLOOM_FILTER_FPP_KEY, String.valueOf(getOrcBloomFilterFpp(tableMetadata.getProperties()))); | |||
| propertiesBuilder.put(TableProperties.ORC_BLOOM_FILTER_COLUMNS, Joiner.on(",").join(columns)); | |||
There was a problem hiding this comment.
io.trino.plugin.iceberg.IcebergTableProperties.ORC_BLOOM_FILTER_COLUMNS
org.apache.iceberg.TableProperties.ORC_BLOOM_FILTER_COLUMNS
We have a name conflict here so we can't static import both.
I thought about refactor the mapping logic between Trino session properties and Iceberg table properties.
The current mapping logic is a bit messy and across multiple classes.
However, the changes will be big so it's better to do it in another PR.
There was a problem hiding this comment.
@oneonestar What do you think about suffixing the ORC Trino session properties with _PROPERTY (as is already done with the other properties) and importing ORC_BLOOM_FILTER_COLUMNS and ORC_BLOOM_FILTER_FPP from org.apache.iceberg.TableProperties into IcebergTableProperties, and then importing them here?
This approach would maintain the pattern that already exists, and it would make things easier for me in the #20393 PR to add the constant that holds the iceberg table properties.
Subsequently, we could revisit how we deal with both kinds of properties.
What are your thoughts on this?
There was a problem hiding this comment.
@ndrluis Thanks for the suggestion.
Rename Trino session properties with _PROPERTY make a lot of sense.
importing ORC_BLOOM_FILTER_COLUMNS and ORC_BLOOM_FILTER_FPP from org.apache.iceberg.TableProperties into IcebergTableProperties, and then importing them here?
I don't understand why these properties are needed in IcebergTableProperties.
I did the change in eddb7e8 . Please take a look.
|
LGTM % comments |
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
oneonestar
force-pushed
the
fix.orc.bloom.filter
branch
2 times, most recently
from
459934b to
eddb7e8
Compare
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergOrcWithBloomFilters.java
Outdated
Show resolved
Hide resolved
oneonestar
force-pushed
the
fix.orc.bloom.filter
branch
3 times, most recently
from
42ffcff to
2e67cda
Compare
oneonestar
force-pushed
the
fix.orc.bloom.filter
branch
from
2e67cda to
56e5374
Compare
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Show resolved
Hide resolved
Description
This PR fix table properties for Iceberg table with orc_bloom_filter:
Fix #20432
Additional context and related issues
The correct table properties should be:
orc.bloom.filter.columns->write.orc.bloom.filter.columnsorc.bloom.filter.fpp->write.orc.bloom.filter.fpp.Spec: https://iceberg.apache.org/docs/latest/configuration/#write-properties
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: