Remove HiveConfig binding from Delta and Iceberg with Glue #19735
Conversation
Before the change `MetastoreHiveStatisticsProvider` was both abstract and concrete class in one: one constructor allowed to plug implementation of the most important method, while the other provided the implementation in a form of a lambda. This made harder to follow the code flow (e.g. finding call sites). The commit splits the class into abstract class (shared logic) and a concrete implementation class.
github-actions
bot
added
tests:hive
iceberg
| @@ -135,6 +137,8 @@ public static Connector createConnector( | |||
| Set<ConnectorTableFunction> connectorTableFunctions = injector.getInstance(Key.get(new TypeLiteral<Set<ConnectorTableFunction>>() {})); | |||
| FunctionProvider functionProvider = injector.getInstance(FunctionProvider.class); | |||
|
|
|||
| checkState(!injector.getBindings().containsKey(Key.get(HiveConfig.class)), "HiveConfig should not be bound"); | |||
There was a problem hiding this comment.
nit: this look over-specific - after all we are not checking other class which we do not expect to be bound if those are not.
There was a problem hiding this comment.
HiveConfig is just the most likely offender (based on history of delta connector), but i am open to suggestoins
Before the change, the Hive Glue metastore wiring would respect the `hive.table-statistics-enabled` configuration property, but not the `statistics_enabled` session property. After the change, handling of stats for Hive Glue metastore is same as for other metastores. That means in particular that some of the `DefaultGlueColumnStatisticsProvider` code will execute implicitly, as not all stats-related operations are guarded by `statistics_enabled` toggle today (see e.g. `SemiTransactionalHiveMetastore` and implicit stats updates after table creation or alteration). This is considered not a blocker.
Delta Lake and Iceberg connectors do not use configuration from `HiveConfig` and so the `HiveConfig`-provided configuration should not be accepted when configuring the connector.
findepi
force-pushed
the
findepi/delta-hive-config
branch
from
89e21a8
to
85049bc
Compare
|
No user facing impact - so no release notes entry @findepi @losipiuk @findinpath ? |
IIUC previously users could set config properties defined in I do not feel release notes are strictly required - for you to decide. |
|
okay ... to be on the safe side we might have to flag it as breaking change and add a release notes entry.. wdyt @martint |
|
Added RN entry. |
|
Doesn't this break the capability for iceberg connector to write to s3? IE: how do you configure writes / credentials etc for iceberg->s3 writes? |
https://github.com/trinodb/trino/blob/79bc8fa09b337ac163582a708b725eb34cf36b0c/lib/trino-hdfs/src/main/java/io/trino/hdfs/s3/HiveS3Config.java (part of Note that this change is about removing binding of |
|
@findinpath we maybe should make sure the Iceberg connector docs links to the Hive S3 docs .. after checking that all properties apply.. |
Delta Lake and Iceberg connectors do not use configuration from
HiveConfigand so theHiveConfig-provided configuration should notbe accepted when configuring the connector.