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.

Sign up for GitHub

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

Jump to bottom

Remove HiveConfig binding from Delta and Iceberg with Glue #19735

Merged
merged 4 commits into from
Nov 15, 2023
Merged

Remove HiveConfig binding from Delta and Iceberg with Glue #19735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4fca0b
Fix typo
findepi Nov 14, 2023
b5238a0
Simplify MetastoreHiveStatisticsProvider code flow
findepi Nov 14, 2023
2fe1d20
Respect Hive statistics_enabled session in Glue metastore
findepi Nov 14, 2023
85049bc
Remove HiveConfig binding from Delta and Iceberg with Glue
findepi Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...delta-lake/src/main/java/io/trino/plugin/deltalake/InternalDeltaLakeConnectorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import io.trino.plugin.base.jmx.MBeanServerModule;
import io.trino.plugin.base.session.SessionPropertiesProvider;
import io.trino.plugin.deltalake.metastore.DeltaLakeMetastoreModule;
import io.trino.plugin.hive.HiveConfig;
import io.trino.plugin.hive.NodeVersion;
import io.trino.spi.NodeManager;
import io.trino.spi.PageIndexerFactory;
Expand All @@ -61,6 +62,7 @@
import java.util.Optional;
import java.util.Set;

import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.inject.multibindings.Multibinder.newSetBinder;

Expand Down Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

HiveConfig is just the most likely offender (based on history of delta connector), but i am open to suggestoins


return new DeltaLakeConnector(
injector,
lifeCycleManager,
Expand Down
5 changes: 5 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1585,6 +1585,8 @@ public ConnectorTableHandle beginStatisticsCollection(ConnectorSession session,
@Override
public void finishStatisticsCollection(ConnectorSession session, ConnectorTableHandle tableHandle, Collection<ComputedStatistics> computedStatistics)
{
verify(isStatisticsEnabled(session), "statistics not enabled");

HiveTableHandle handle = (HiveTableHandle) tableHandle;
SchemaTableName tableName = handle.getSchemaTableName();
Table table = metastore.getTable(tableName.getSchemaName(), tableName.getTableName())
Expand Down Expand Up @@ -3453,6 +3455,9 @@ public TableStatisticsMetadata getStatisticsCollectionMetadataForWrite(Connector
if (!isCollectColumnStatisticsOnWrite(session)) {
return TableStatisticsMetadata.empty();
}
if (!isStatisticsEnabled(session)) {
throw new TrinoException(NOT_SUPPORTED, "Table statistics must be enabled when column statistics collection on write is enabled");
}
if (isTransactional(tableMetadata.getProperties()).orElse(false)) {
// TODO(https://github.com/trinodb/trino/issues/1956) updating table statistics for transactional not supported right now.
return TableStatisticsMetadata.empty();
Expand Down
69 changes: 0 additions & 69 deletions ...c/main/java/io/trino/plugin/hive/metastore/glue/DisabledGlueColumnStatisticsProvider.java
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions ...java/io/trino/plugin/hive/metastore/glue/DisabledGlueColumnStatisticsProviderFactory.java
View file
Open in desktop

This file was deleted.

20 changes: 2 additions & 18 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueMetastoreModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.amazonaws.services.glue.model.Table;
import com.google.inject.Binder;
import com.google.inject.Key;
import com.google.inject.Module;
import com.google.inject.Provides;
import com.google.inject.Scopes;
import com.google.inject.Singleton;
Expand All @@ -31,7 +30,6 @@
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.instrumentation.awssdk.v1_11.AwsSdkTelemetry;
import io.trino.plugin.hive.AllowHiveTableRename;
import io.trino.plugin.hive.HiveConfig;
import io.trino.plugin.hive.metastore.HiveMetastoreFactory;
import io.trino.plugin.hive.metastore.RawHiveMetastoreFactory;

Expand All @@ -42,8 +40,6 @@
import static com.google.inject.multibindings.Multibinder.newSetBinder;
import static com.google.inject.multibindings.OptionalBinder.newOptionalBinder;
import static io.airlift.concurrent.Threads.daemonThreadsNamed;
import static io.airlift.configuration.ConditionalModule.conditionalModule;
import static io.airlift.configuration.ConfigBinder.configBinder;
import static java.util.concurrent.Executors.newCachedThreadPool;
import static org.weakref.jmx.guice.ExportBinder.newExporter;

Expand All @@ -58,7 +54,6 @@ protected void setup(Binder binder)
glueConfig.getCatalogId().ifPresent(catalogId -> requestHandlers.addBinding().toInstance(new GlueCatalogIdRequestHandler(catalogId)));
glueConfig.getGlueProxyApiId().ifPresent(glueProxyApiId -> requestHandlers.addBinding()
.toInstance(new ProxyApiRequestHandler(glueProxyApiId)));
configBinder(binder).bindConfig(HiveConfig.class);
binder.bind(AWSCredentialsProvider.class).toProvider(GlueCredentialsProvider.class).in(Scopes.SINGLETON);

newOptionalBinder(binder, Key.get(new TypeLiteral<Predicate<Table>>() {}, ForGlueHiveMetastore.class))
Expand All @@ -78,19 +73,8 @@ protected void setup(Binder binder)

binder.bind(Key.get(boolean.class, AllowHiveTableRename.class)).toInstance(false);

install(conditionalModule(
HiveConfig.class,
HiveConfig::isTableStatisticsEnabled,
getGlueStatisticsModule(DefaultGlueColumnStatisticsProviderFactory.class),
getGlueStatisticsModule(DisabledGlueColumnStatisticsProviderFactory.class)));
}

private Module getGlueStatisticsModule(Class<? extends GlueColumnStatisticsProviderFactory> statisticsPrividerFactoryClass)
{
return internalBinder -> newOptionalBinder(internalBinder, GlueColumnStatisticsProviderFactory.class)
.setDefault()
.to(statisticsPrividerFactoryClass)
.in(Scopes.SINGLETON);
newOptionalBinder(binder, GlueColumnStatisticsProviderFactory.class)
.setDefault().to(DefaultGlueColumnStatisticsProviderFactory.class).in(Scopes.SINGLETON);
}

@ProvidesIntoSet
Expand Down