Skip to content

Commit

Permalink
Remove kill-switch to disable automatic fetch-size selection
Browse files Browse the repository at this point in the history 
The configuration toggles `xxx.disable-automatic-fetch-size` were
introduced only temporarily, as a kill-switch.
  • Loading branch information
@findepi
findepi committed May 5, 2023
1 parent f202784 commit 58d44a2
Show file tree
Hide file tree
Showing 9 changed files with 9 additions and 72 deletions.
7 changes: 1 addition & 6 deletions plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,6 @@ public class OracleClient
.put(TIMESTAMP_TZ_MILLIS, WriteMapping.longMapping("timestamp(3) with time zone", oracleTimestampWithTimeZoneWriteFunction()))
.buildOrThrow();

private final boolean disableAutomaticFetchSize;
private final boolean synonymsEnabled;
private final ConnectorExpressionRewriter<ParameterizedExpression> connectorExpressionRewriter;
private final AggregateFunctionRewriter<JdbcExpression, ?> aggregateFunctionRewriter;
Expand All @@ -212,7 +211,6 @@ public OracleClient(
{
super("\"", connectionFactory, queryBuilder, config.getJdbcTypesMappedToVarchar(), identifierMapping, queryModifier, false);

this.disableAutomaticFetchSize = oracleConfig.isDisableAutomaticFetchSize();
this.synonymsEnabled = oracleConfig.isSynonymsEnabled();

this.connectorExpressionRewriter = JdbcConnectorExpressionRewriterBuilder.newBuilder()
Expand Down Expand Up @@ -262,12 +260,9 @@ public PreparedStatement getPreparedStatement(Connection connection, String sql,
throws SQLException
{
PreparedStatement statement = connection.prepareStatement(sql);
if (disableAutomaticFetchSize) {
statement.setFetchSize(1000);
}
// This is a heuristic, not exact science. A better formula can perhaps be found with measurements.
// Column count is not known for non-SELECT queries. Not setting fetch size for these.
else if (columnCount.isPresent()) {
if (columnCount.isPresent()) {
statement.setFetchSize(max(100_000 / columnCount.get(), 1_000));
}
return statement;
Expand Down
17 changes: 2 additions & 15 deletions plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import io.airlift.configuration.Config;
import io.airlift.configuration.ConfigDescription;
import io.airlift.configuration.DefunctConfig;
import io.airlift.units.Duration;

import javax.validation.constraints.AssertTrue;
Expand All @@ -27,9 +28,9 @@

import static java.util.concurrent.TimeUnit.MINUTES;

@DefunctConfig("oracle.disable-automatic-fetch-size")
public class OracleConfig
{
private boolean disableAutomaticFetchSize;
private boolean synonymsEnabled;
private boolean remarksReportingEnabled;
private Integer defaultNumberScale;
Expand All @@ -39,20 +40,6 @@ public class OracleConfig
private int connectionPoolMaxSize = 30;
private Duration inactiveConnectionTimeout = new Duration(20, MINUTES);

@Deprecated
public boolean isDisableAutomaticFetchSize()
{
return disableAutomaticFetchSize;
}

@Deprecated // TODO temporary kill-switch, to be removed
@Config("oracle.disable-automatic-fetch-size")
public OracleConfig setDisableAutomaticFetchSize(boolean disableAutomaticFetchSize)
{
this.disableAutomaticFetchSize = disableAutomaticFetchSize;
return this;
}

@NotNull
public boolean isSynonymsEnabled()
{
Expand Down
3 changes: 0 additions & 3 deletions plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOracleConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public class TestOracleConfig
public void testDefaults()
{
assertRecordedDefaults(recordDefaults(OracleConfig.class)
.setDisableAutomaticFetchSize(false)
.setSynonymsEnabled(false)
.setRemarksReportingEnabled(false)
.setDefaultNumberScale(null)
Expand All @@ -51,7 +50,6 @@ public void testDefaults()
public void testExplicitPropertyMappings()
{
Map<String, String> properties = ImmutableMap.<String, String>builder()
.put("oracle.disable-automatic-fetch-size", "true")
.put("oracle.synonyms.enabled", "true")
.put("oracle.remarks-reporting.enabled", "true")
.put("oracle.number.default-scale", "2")
Expand All @@ -63,7 +61,6 @@ public void testExplicitPropertyMappings()
.buildOrThrow();

OracleConfig expected = new OracleConfig()
.setDisableAutomaticFetchSize(true)
.setSynonymsEnabled(true)
.setRemarksReportingEnabled(true)
.setDefaultNumberScale(2)
Expand Down
7 changes: 1 addition & 6 deletions plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,6 @@ public class PostgreSqlClient
return FULL_PUSHDOWN.apply(session, simplifiedDomain);
};

private final boolean disableAutomaticFetchSize;
private final Type jsonType;
private final Type uuidType;
private final MapType varcharMapType;
Expand All @@ -290,7 +289,6 @@ public PostgreSqlClient(
RemoteQueryModifier queryModifier)
{
super("\"", connectionFactory, queryBuilder, config.getJdbcTypesMappedToVarchar(), identifierMapping, queryModifier, true);
this.disableAutomaticFetchSize = postgreSqlConfig.isDisableAutomaticFetchSize();
this.jsonType = typeManager.getType(new TypeSignature(JSON));
this.uuidType = typeManager.getType(new TypeSignature(StandardTypes.UUID));
this.varcharMapType = (MapType) typeManager.getType(mapType(VARCHAR.getTypeSignature(), VARCHAR.getTypeSignature()));
Expand Down Expand Up @@ -408,12 +406,9 @@ public PreparedStatement getPreparedStatement(Connection connection, String sql,
// fetch-size is ignored when connection is in auto-commit
connection.setAutoCommit(false);
PreparedStatement statement = connection.prepareStatement(sql);
if (disableAutomaticFetchSize) {
statement.setFetchSize(1000);
}
// This is a heuristic, not exact science. A better formula can perhaps be found with measurements.
// Column count is not known for non-SELECT queries. Not setting fetch size for these.
else if (columnCount.isPresent()) {
if (columnCount.isPresent()) {
statement.setFetchSize(max(100_000 / columnCount.get(), 1_000));
}
return statement;
Expand Down
17 changes: 2 additions & 15 deletions plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,18 @@
package io.trino.plugin.postgresql;

import io.airlift.configuration.Config;
import io.airlift.configuration.DefunctConfig;
import io.airlift.configuration.LegacyConfig;

import javax.validation.constraints.NotNull;

@DefunctConfig("postgresql.disable-automatic-fetch-size")
public class PostgreSqlConfig
{
private boolean disableAutomaticFetchSize;
private ArrayMapping arrayMapping = ArrayMapping.DISABLED;
private boolean includeSystemTables;
private boolean enableStringPushdownWithCollate;

@Deprecated
public boolean isDisableAutomaticFetchSize()
{
return disableAutomaticFetchSize;
}

@Deprecated // TODO temporary kill-switch, to be removed
@Config("postgresql.disable-automatic-fetch-size")
public PostgreSqlConfig setDisableAutomaticFetchSize(boolean disableAutomaticFetchSize)
{
this.disableAutomaticFetchSize = disableAutomaticFetchSize;
return this;
}

public enum ArrayMapping
{
DISABLED,
Expand Down
3 changes: 0 additions & 3 deletions plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ public class TestPostgreSqlConfig
public void testDefaults()
{
assertRecordedDefaults(recordDefaults(PostgreSqlConfig.class)
.setDisableAutomaticFetchSize(false)
.setArrayMapping(PostgreSqlConfig.ArrayMapping.DISABLED)
.setIncludeSystemTables(false)
.setEnableStringPushdownWithCollate(false));
Expand All @@ -38,14 +37,12 @@ public void testDefaults()
public void testExplicitPropertyMappings()
{
Map<String, String> properties = ImmutableMap.<String, String>builder()
.put("postgresql.disable-automatic-fetch-size", "true")
.put("postgresql.array-mapping", "AS_ARRAY")
.put("postgresql.include-system-tables", "true")
.put("postgresql.experimental.enable-string-pushdown-with-collate", "true")
.buildOrThrow();

PostgreSqlConfig expected = new PostgreSqlConfig()
.setDisableAutomaticFetchSize(true)
.setArrayMapping(PostgreSqlConfig.ArrayMapping.AS_ARRAY)
.setIncludeSystemTables(true)
.setEnableStringPushdownWithCollate(true);
Expand Down
7 changes: 1 addition & 6 deletions plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,6 @@ public class RedshiftClient
.toFormatter();
private static final OffsetDateTime REDSHIFT_MIN_SUPPORTED_TIMESTAMP_TZ = OffsetDateTime.of(-4712, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);

private final boolean disableAutomaticFetchSize;
private final AggregateFunctionRewriter<JdbcExpression, ?> aggregateFunctionRewriter;
private final boolean statisticsEnabled;
private final RedshiftTableStatisticsReader statisticsReader;
Expand All @@ -244,7 +243,6 @@ public RedshiftClient(
RedshiftConfig redshiftConfig)
{
super("\"", connectionFactory, queryBuilder, config.getJdbcTypesMappedToVarchar(), identifierMapping, queryModifier, true);
this.disableAutomaticFetchSize = redshiftConfig.isDisableAutomaticFetchSize();
this.legacyTypeMapping = redshiftConfig.isLegacyTypeMapping();
ConnectorExpressionRewriter<ParameterizedExpression> connectorExpressionRewriter = JdbcConnectorExpressionRewriterBuilder.newBuilder()
.addStandardRules(this::quoted)
Expand Down Expand Up @@ -415,12 +413,9 @@ public PreparedStatement getPreparedStatement(Connection connection, String sql,
// that.
connection.setAutoCommit(false);
PreparedStatement statement = connection.prepareStatement(sql);
if (disableAutomaticFetchSize) {
statement.setFetchSize(1000);
}
// This is a heuristic, not exact science. A better formula can perhaps be found with measurements.
// Column count is not known for non-SELECT queries. Not setting fetch size for these.
else if (columnCount.isPresent()) {
if (columnCount.isPresent()) {
statement.setFetchSize(max(100_000 / columnCount.get(), 1_000));
}
return statement;
Expand Down
17 changes: 2 additions & 15 deletions plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,13 @@
package io.trino.plugin.redshift;

import io.airlift.configuration.Config;
import io.airlift.configuration.DefunctConfig;

@DefunctConfig("redshift.disable-automatic-fetch-size")
public class RedshiftConfig
{
private boolean disableAutomaticFetchSize;
private boolean legacyTypeMapping;

@Deprecated
public boolean isDisableAutomaticFetchSize()
{
return disableAutomaticFetchSize;
}

@Deprecated // TODO temporary kill-switch, to be removed
@Config("redshift.disable-automatic-fetch-size")
public RedshiftConfig setDisableAutomaticFetchSize(boolean disableAutomaticFetchSize)
{
this.disableAutomaticFetchSize = disableAutomaticFetchSize;
return this;
}

public boolean isLegacyTypeMapping()
{
return legacyTypeMapping;
Expand Down
3 changes: 0 additions & 3 deletions plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,17 @@ public class TestRedshiftConfig
public void testDefaults()
{
assertRecordedDefaults(recordDefaults(RedshiftConfig.class)
.setDisableAutomaticFetchSize(false)
.setLegacyTypeMapping(false));
}

@Test
public void testExplicitPropertyMappings()
{
Map<String, String> properties = ImmutableMap.<String, String>builder()
.put("redshift.disable-automatic-fetch-size", "true")
.put("redshift.use-legacy-type-mapping", "true")
.buildOrThrow();

RedshiftConfig expected = new RedshiftConfig()
.setDisableAutomaticFetchSize(true)
.setLegacyTypeMapping(true);

assertFullMapping(properties, expected);
Expand Down

0 comments on commit 58d44a2

Please sign in to comment.