Follow-up code review

trinodb · Sep 8, 2023 · 2ee0b52 · 2ee0b52
1 parent 4346680
commit 2ee0b52
Show file tree

Hide file tree

Showing 13 changed files with 76 additions and 114 deletions.
diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
@@ -113,17 +113,17 @@ public class DefaultJdbcMetadata
 
     private final AtomicReference<Runnable> rollbackAction = new AtomicReference<>();
 
-    private final ColumnWithAliasFormatter aliasFormatter;
+    private final SyntheticColumnHandleBuilder syntheticColumnBuilder;
 
     public DefaultJdbcMetadata(JdbcClient jdbcClient,
             boolean precalculateStatisticsForPushdown,
             Set<JdbcQueryEventListener> jdbcQueryEventListeners,
-            ColumnWithAliasFormatter aliasFormatter)
+            SyntheticColumnHandleBuilder syntheticColumnBuilder)
     {
         this.jdbcClient = requireNonNull(jdbcClient, "jdbcClient is null");
         this.precalculateStatisticsForPushdown = precalculateStatisticsForPushdown;
         this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "queryEventListeners is null"));
-        this.aliasFormatter = requireNonNull(aliasFormatter, "aliasFormatter is null");
+        this.syntheticColumnBuilder = requireNonNull(syntheticColumnBuilder, "syntheticColumnBuilder is null");
     }
 
     @Override
@@ -459,14 +459,14 @@ public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(
 
         ImmutableMap.Builder<JdbcColumnHandle, JdbcColumnHandle> newLeftColumnsBuilder = ImmutableMap.builder();
         for (JdbcColumnHandle column : jdbcClient.getColumns(session, leftHandle)) {
-            newLeftColumnsBuilder.put(column, aliasFormatter.format(session, column, nextSyntheticColumnId));
+            newLeftColumnsBuilder.put(column, syntheticColumnBuilder.get(column, nextSyntheticColumnId));
             nextSyntheticColumnId++;
         }
         Map<JdbcColumnHandle, JdbcColumnHandle> newLeftColumns = newLeftColumnsBuilder.buildOrThrow();
 
         ImmutableMap.Builder<JdbcColumnHandle, JdbcColumnHandle> newRightColumnsBuilder = ImmutableMap.builder();
         for (JdbcColumnHandle column : jdbcClient.getColumns(session, rightHandle)) {
-            newRightColumnsBuilder.put(column, aliasFormatter.format(session, column, nextSyntheticColumnId));
+            newRightColumnsBuilder.put(column, syntheticColumnBuilder.get(column, nextSyntheticColumnId));
             nextSyntheticColumnId++;
         }
         Map<JdbcColumnHandle, JdbcColumnHandle> newRightColumns = newRightColumnsBuilder.buildOrThrow();

diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java
@@ -28,14 +28,14 @@ public class DefaultJdbcMetadataFactory
     private final JdbcClient jdbcClient;
     private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;
 
-    protected final ColumnWithAliasFormatter aliasFormatter;
+    protected final SyntheticColumnHandleBuilder syntheticColumnBuilder;
 
     @Inject
-    public DefaultJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners, ColumnWithAliasFormatter aliasFormatter)
+    public DefaultJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners, SyntheticColumnHandleBuilder syntheticColumnBuilder)
     {
         this.jdbcClient = requireNonNull(jdbcClient, "jdbcClient is null");
         this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "queryEventListeners is null"));
-        this.aliasFormatter = requireNonNull(aliasFormatter, "aliasFormatter is null");
+        this.syntheticColumnBuilder = requireNonNull(syntheticColumnBuilder, "syntheticColumnBuilder is null");
     }
 
     @Override
@@ -54,6 +54,6 @@ public JdbcMetadata create(JdbcTransactionHandle transaction)
 
     protected JdbcMetadata create(JdbcClient transactionCachingJdbcClient)
     {
-        return new DefaultJdbcMetadata(transactionCachingJdbcClient, true, jdbcQueryEventListeners, aliasFormatter);
+        return new DefaultJdbcMetadata(transactionCachingJdbcClient, true, jdbcQueryEventListeners, syntheticColumnBuilder);
     }
 }
diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java
@@ -50,7 +50,7 @@ public void setup(Binder binder)
         install(new JdbcDiagnosticModule());
         install(new IdentifierMappingModule());
         install(new RemoteQueryModifierModule());
-        install(new ColumnWithAliasFormatterModule());
+        install(new SyntheticColumnHandleBuilderModule());
 
         newOptionalBinder(binder, ConnectorAccessControl.class);
         newOptionalBinder(binder, QueryBuilder.class).setDefault().to(DefaultQueryBuilder.class).in(Scopes.SINGLETON);

diff --git a/...plugin/jdbc/ColumnWithAliasFormatter.java → ...in/jdbc/SyntheticColumnHandleBuilder.java b/...plugin/jdbc/ColumnWithAliasFormatter.java → ...in/jdbc/SyntheticColumnHandleBuilder.java
@@ -13,26 +13,25 @@
  */
 package io.trino.plugin.jdbc;
 
-import io.trino.spi.connector.ConnectorSession;
-
 import static com.google.common.base.Splitter.fixedLength;
-import static com.google.common.base.Strings.padStart;
+import static com.google.common.base.Verify.verify;
 
-public class ColumnWithAliasFormatter
+public class SyntheticColumnHandleBuilder
 {
     public static final int DEFAULT_COLUMN_ALIAS_LENGTH = 30;
-    public static final int ORIGINAL_COLUMN_NAME_LENGTH = 24;
 
-    public JdbcColumnHandle format(ConnectorSession session, JdbcColumnHandle column, int nextSyntheticColumnId)
+    public JdbcColumnHandle get(JdbcColumnHandle column, int nextSyntheticColumnId)
     {
-        int sequentialNumberLength = DEFAULT_COLUMN_ALIAS_LENGTH - ORIGINAL_COLUMN_NAME_LENGTH - 1;
+        verify(nextSyntheticColumnId >= 0, "nextSyntheticColumnId rolled over and is not monotonically increasing any more");
+
+        int sequentialNumberLength = String.valueOf(nextSyntheticColumnId).length();
+        int originalColumnNameLength = DEFAULT_COLUMN_ALIAS_LENGTH - sequentialNumberLength - "_".length();
 
-        String originalColumnNameTruncated = fixedLength(ORIGINAL_COLUMN_NAME_LENGTH)
+        String columnNameTruncated = fixedLength(originalColumnNameLength)
                 .split(column.getColumnName())
                 .iterator()
                 .next();
-        String formatString = "%s_%0" + sequentialNumberLength + "d";
-        String columnName = originalColumnNameTruncated + "_" + padStart(Integer.toString(nextSyntheticColumnId), sequentialNumberLength, '0');
+        String columnName = columnNameTruncated + "_" + nextSyntheticColumnId;
         return JdbcColumnHandle.builderFrom(column)
                 .setColumnName(columnName)
                 .build();

diff --git a/.../jdbc/ColumnWithAliasFormatterModule.java → ...c/SyntheticColumnHandleBuilderModule.java b/.../jdbc/ColumnWithAliasFormatterModule.java → ...c/SyntheticColumnHandleBuilderModule.java
@@ -16,12 +16,12 @@
 import com.google.inject.AbstractModule;
 import com.google.inject.Singleton;
 
-public class ColumnWithAliasFormatterModule
+public class SyntheticColumnHandleBuilderModule
         extends AbstractModule
 {
     @Override
     public void configure()
     {
-        bind(ColumnWithAliasFormatter.class).in(Singleton.class);
+        bind(SyntheticColumnHandleBuilder.class).in(Singleton.class);
     }
 }
diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestDefaultJdbcMetadata.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestDefaultJdbcMetadata.java
@@ -16,7 +16,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.inject.Inject;
 import io.trino.spi.connector.AggregateFunction;
 import io.trino.spi.connector.AggregationApplicationResult;
 import io.trino.spi.connector.ColumnHandle;
@@ -35,7 +34,6 @@
 import io.trino.testing.TestingConnectorSession;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeMethod;
-import org.testng.annotations.Guice;
 import org.testng.annotations.Test;
 
 import java.util.List;
@@ -56,15 +54,13 @@
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 @Test(singleThreaded = true)
-@Guice(modules = ColumnWithAliasFormatterModule.class)
 public class TestDefaultJdbcMetadata
 {
     private TestingDatabase database;
     private DefaultJdbcMetadata metadata;
     private JdbcTableHandle tableHandle;
 
-    @Inject
-    private ColumnWithAliasFormatter aliasFormatter;
+    private final SyntheticColumnHandleBuilder syntheticColumnHandleBuilder = new SyntheticColumnHandleBuilder();
 
     @BeforeMethod
     public void setUp()
@@ -75,7 +71,7 @@ public void setUp()
                 Optional.empty()),
                 false,
                 ImmutableSet.of(),
-                aliasFormatter);
+                syntheticColumnHandleBuilder);
         tableHandle = metadata.getTableHandle(SESSION, new SchemaTableName("example", "numbers"));
     }
 
@@ -86,7 +82,7 @@ public void testSupportsRetriesValidation()
                 Optional.of(false)),
                 false,
                 ImmutableSet.of(),
-                aliasFormatter);
+                syntheticColumnHandleBuilder);
         ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());
 
         assertThatThrownBy(() -> {
@@ -105,7 +101,7 @@ public void testNonTransactionalInsertValidation()
                 Optional.of(true)),
                 false,
                 ImmutableSet.of(),
-                aliasFormatter);
+                syntheticColumnHandleBuilder);
         ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());
 
         ConnectorSession session = TestingConnectorSession.builder()

diff --git a/...in/jdbc/TestColumnWithAliasFormatter.java → ...dbc/TestSyntheticColumnHandleBuilder.java b/...in/jdbc/TestColumnWithAliasFormatter.java → ...dbc/TestSyntheticColumnHandleBuilder.java
@@ -13,45 +13,50 @@
  */
 package io.trino.plugin.jdbc;
 
-import com.google.inject.Inject;
-import io.trino.testing.TestingConnectorSession;
-import org.testng.annotations.Guice;
+import com.google.common.base.VerifyException;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_VARCHAR;
 import static io.trino.spi.type.VarcharType.VARCHAR;
+import static java.lang.Integer.MAX_VALUE;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
-@Guice(modules = ColumnWithAliasFormatterModule.class)
-public class TestColumnWithAliasFormatter
+public class TestSyntheticColumnHandleBuilder
 {
-    @Inject
-    private ColumnWithAliasFormatter actor;
+    private final SyntheticColumnHandleBuilder syntheticColumnHandleBuilder = new SyntheticColumnHandleBuilder();
 
-    private final TestingConnectorSession session = TestingConnectorSession.builder().build();
+    @DataProvider(name = "columns")
+    public static Object[][] testData()
+    {
+        return new Object[][] {
+                {"column_0", 999, "column_0_999"},
+                {"column_with_over_twenty_characters", 100, "column_with_over_twenty_ch_100"},
+                {"column_with_over_twenty_characters", MAX_VALUE, "column_with_over_tw_2147483647"}
+        };
+    }
 
-    @Test
-    public void testTooLongName()
+    @Test(dataProvider = "columns")
+    public void testColumnAliasTruncation(String columnName, int nextSynthenticId, String expectedSyntheticColumnName)
     {
         JdbcColumnHandle column = getDefaultColumnHandleBuilder()
-                .setColumnName("column_with_over_twenty_characters")
+                .setColumnName(columnName)
                 .build();
 
-        JdbcColumnHandle result = actor.format(session, column, 100);
+        JdbcColumnHandle result = syntheticColumnHandleBuilder.get(column, nextSynthenticId);
 
-        assertThat(result.getColumnName()).isEqualTo("column_with_over_twenty__00100");
+        assertThat(result.getColumnName()).isEqualTo(expectedSyntheticColumnName);
     }
 
     @Test
-    public void testTooShortName()
+    public void testNegativeSyntheticId()
     {
         JdbcColumnHandle column = getDefaultColumnHandleBuilder()
                 .setColumnName("column_0")
                 .build();
 
-        JdbcColumnHandle result = actor.format(session, column, 999);
-
-        assertThat(result.getColumnName()).isEqualTo("column_0_00999");
+        assertThatThrownBy(() -> syntheticColumnHandleBuilder.get(column, -2147483648)).isInstanceOf(VerifyException.class);
     }
 
     private static JdbcColumnHandle.Builder getDefaultColumnHandleBuilder()

diff --git a/plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteJdbcMetadataFactory.java b/plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteJdbcMetadataFactory.java
@@ -15,11 +15,11 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.inject.Inject;
-import io.trino.plugin.jdbc.ColumnWithAliasFormatter;
 import io.trino.plugin.jdbc.DefaultJdbcMetadataFactory;
 import io.trino.plugin.jdbc.JdbcClient;
 import io.trino.plugin.jdbc.JdbcMetadata;
 import io.trino.plugin.jdbc.JdbcQueryEventListener;
+import io.trino.plugin.jdbc.SyntheticColumnHandleBuilder;
 
 import java.util.Set;
 
@@ -31,16 +31,17 @@ public class IgniteJdbcMetadataFactory
     private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;
 
     @Inject
-    public IgniteJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners,
-            ColumnWithAliasFormatter aliasFormatter)
+    public IgniteJdbcMetadataFactory(JdbcClient jdbcClient,
+                Set<JdbcQueryEventListener> jdbcQueryEventListeners,
+                SyntheticColumnHandleBuilder syntheticColumnHandleBuilder)
     {
-        super(jdbcClient, jdbcQueryEventListeners, aliasFormatter);
+        super(jdbcClient, jdbcQueryEventListeners, syntheticColumnHandleBuilder);
         this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "jdbcQueryEventListeners is null"));
     }
 
     @Override
     protected JdbcMetadata create(JdbcClient transactionCachingJdbcClient)
     {
-        return new IgniteMetadata(transactionCachingJdbcClient, jdbcQueryEventListeners, aliasFormatter);
+        return new IgniteMetadata(transactionCachingJdbcClient, jdbcQueryEventListeners, syntheticColumnBuilder);
     }
 }
diff --git a/plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java b/plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java
@@ -16,7 +16,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.inject.Inject;
 import io.airlift.slice.Slice;
-import io.trino.plugin.jdbc.ColumnWithAliasFormatter;
 import io.trino.plugin.jdbc.DefaultJdbcMetadata;
 import io.trino.plugin.jdbc.JdbcClient;
 import io.trino.plugin.jdbc.JdbcColumnHandle;
@@ -25,6 +24,7 @@
 import io.trino.plugin.jdbc.JdbcTableHandle;
 import io.trino.plugin.jdbc.JdbcTypeHandle;
 import io.trino.plugin.jdbc.RemoteTableName;
+import io.trino.plugin.jdbc.SyntheticColumnHandleBuilder;
 import io.trino.spi.TrinoException;
 import io.trino.spi.connector.ColumnHandle;
 import io.trino.spi.connector.ColumnMetadata;
@@ -58,10 +58,11 @@ public class IgniteMetadata
     private final JdbcClient igniteClient;
 
     @Inject
-    public IgniteMetadata(JdbcClient igniteClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners,
-            ColumnWithAliasFormatter aliasFormatter)
+    public IgniteMetadata(JdbcClient igniteClient,
+                Set<JdbcQueryEventListener> jdbcQueryEventListeners,
+                SyntheticColumnHandleBuilder syntheticColumnHandleBuilder)
     {
-        super(igniteClient, false, jdbcQueryEventListeners, aliasFormatter);
+        super(igniteClient, false, jdbcQueryEventListeners, syntheticColumnHandleBuilder);
         this.igniteClient = requireNonNull(igniteClient, "igniteClient is null");
     }