Truncate synthetic column names during join pushdown if needed

In some operations (`applyAggregation` and `applyJoin`) synthetic columns are generated which get pushed down as projections into the remote database. It's possible for the generated column names to exceed the valid identifier length supported by the remote database leading to query failures or silent truncation of column name leading to unpredictable behaviour. For example on Oracle such a failure manifests as: ORA-00972: identifier is too long This change truncates the synthetic column names to always be smaller than the limit for Oracle. Also note that this change just fixes the `applyJoin` path since that's the one where the issue was observed. In future the limit will be allowed to vary according to database being used instead of hardcoded to the value for Oracle.
trinodb · Sep 10, 2023 · 5e520fc · 5e520fc
1 parent 3ec5767
commit 5e520fc
Show file tree

Hide file tree

Showing 12 changed files with 202 additions and 19 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,11 +113,17 @@ public class DefaultJdbcMetadata
 
     private final AtomicReference<Runnable> rollbackAction = new AtomicReference<>();
 
-    public DefaultJdbcMetadata(JdbcClient jdbcClient, boolean precalculateStatisticsForPushdown, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
+    private final SyntheticColumnHandleBuilder syntheticColumnBuilder;
+
+    public DefaultJdbcMetadata(JdbcClient jdbcClient,
+            boolean precalculateStatisticsForPushdown,
+            Set<JdbcQueryEventListener> jdbcQueryEventListeners,
+            SyntheticColumnHandleBuilder syntheticColumnBuilder)
     {
         this.jdbcClient = requireNonNull(jdbcClient, "jdbcClient is null");
         this.precalculateStatisticsForPushdown = precalculateStatisticsForPushdown;
         this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "queryEventListeners is null"));
+        this.syntheticColumnBuilder = requireNonNull(syntheticColumnBuilder, "syntheticColumnBuilder is null");
     }
 
     @Override
@@ -453,18 +459,14 @@ public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(
 
         ImmutableMap.Builder<JdbcColumnHandle, JdbcColumnHandle> newLeftColumnsBuilder = ImmutableMap.builder();
         for (JdbcColumnHandle column : jdbcClient.getColumns(session, leftHandle)) {
-            newLeftColumnsBuilder.put(column, JdbcColumnHandle.builderFrom(column)
-                    .setColumnName(column.getColumnName() + "_" + nextSyntheticColumnId)
-                    .build());
+            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, JdbcColumnHandle.builderFrom(column)
-                    .setColumnName(column.getColumnName() + "_" + nextSyntheticColumnId)
-                    .build());
+            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,11 +28,14 @@ public class DefaultJdbcMetadataFactory
     private final JdbcClient jdbcClient;
     private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;
 
+    protected final SyntheticColumnHandleBuilder syntheticColumnBuilder;
+
     @Inject
-    public DefaultJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
+    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.syntheticColumnBuilder = requireNonNull(syntheticColumnBuilder, "syntheticColumnBuilder is null");
     }
 
     @Override
@@ -51,6 +54,6 @@ public JdbcMetadata create(JdbcTransactionHandle transaction)
 
     protected JdbcMetadata create(JdbcClient transactionCachingJdbcClient)
     {
-        return new DefaultJdbcMetadata(transactionCachingJdbcClient, true, jdbcQueryEventListeners);
+        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,6 +50,7 @@ public void setup(Binder binder)
         install(new JdbcDiagnosticModule());
         install(new IdentifierMappingModule());
         install(new RemoteQueryModifierModule());
+        install(new SyntheticColumnHandleBuilderModule());
 
         newOptionalBinder(binder, ConnectorAccessControl.class);
         newOptionalBinder(binder, QueryBuilder.class).setDefault().to(DefaultQueryBuilder.class).in(Scopes.SINGLETON);

diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/SyntheticColumnHandleBuilder.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/SyntheticColumnHandleBuilder.java
@@ -0,0 +1,39 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.jdbc;
+
+import static com.google.common.base.Splitter.fixedLength;
+import static com.google.common.base.Verify.verify;
+
+public class SyntheticColumnHandleBuilder
+{
+    public static final int DEFAULT_COLUMN_ALIAS_LENGTH = 30;
+
+    public JdbcColumnHandle get(JdbcColumnHandle column, int nextSyntheticColumnId)
+    {
+        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 columnNameTruncated = fixedLength(originalColumnNameLength)
+                .split(column.getColumnName())
+                .iterator()
+                .next();
+        String columnName = columnNameTruncated + "_" + nextSyntheticColumnId;
+        return JdbcColumnHandle.builderFrom(column)
+                .setColumnName(columnName)
+                .build();
+    }
+}
diff --git a/...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/SyntheticColumnHandleBuilderModule.java b/...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/SyntheticColumnHandleBuilderModule.java
@@ -0,0 +1,27 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.jdbc;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.Singleton;
+
+public class SyntheticColumnHandleBuilderModule
+        extends AbstractModule
+{
+    @Override
+    public void configure()
+    {
+        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
@@ -60,19 +60,29 @@ public class TestDefaultJdbcMetadata
     private DefaultJdbcMetadata metadata;
     private JdbcTableHandle tableHandle;
 
+    private final SyntheticColumnHandleBuilder syntheticColumnHandleBuilder = new SyntheticColumnHandleBuilder();
+
     @BeforeMethod
     public void setUp()
             throws Exception
     {
         database = new TestingDatabase();
-        metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(), Optional.empty()), false, ImmutableSet.of());
+        metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(),
+                Optional.empty()),
+                false,
+                ImmutableSet.of(),
+                syntheticColumnHandleBuilder);
         tableHandle = metadata.getTableHandle(SESSION, new SchemaTableName("example", "numbers"));
     }
 
     @Test
     public void testSupportsRetriesValidation()
     {
-        metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(), Optional.of(false)), false, ImmutableSet.of());
+        metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(),
+                Optional.of(false)),
+                false,
+                ImmutableSet.of(),
+                syntheticColumnHandleBuilder);
         ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());
 
         assertThatThrownBy(() -> {
@@ -87,7 +97,11 @@ public void testSupportsRetriesValidation()
     @Test
     public void testNonTransactionalInsertValidation()
     {
-        metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(), Optional.of(true)), false, ImmutableSet.of());
+        metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(),
+                Optional.of(true)),
+                false,
+                ImmutableSet.of(),
+                syntheticColumnHandleBuilder);
         ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());
 
         ConnectorSession session = TestingConnectorSession.builder()

diff --git a/.../trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestSyntheticColumnHandleBuilder.java b/.../trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestSyntheticColumnHandleBuilder.java
@@ -0,0 +1,68 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.jdbc;
+
+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;
+
+public class TestSyntheticColumnHandleBuilder
+{
+    private final SyntheticColumnHandleBuilder syntheticColumnHandleBuilder = new SyntheticColumnHandleBuilder();
+
+    @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(dataProvider = "columns")
+    public void testColumnAliasTruncation(String columnName, int nextSynthenticId, String expectedSyntheticColumnName)
+    {
+        JdbcColumnHandle column = getDefaultColumnHandleBuilder()
+                .setColumnName(columnName)
+                .build();
+
+        JdbcColumnHandle result = syntheticColumnHandleBuilder.get(column, nextSynthenticId);
+
+        assertThat(result.getColumnName()).isEqualTo(expectedSyntheticColumnName);
+    }
+
+    @Test
+    public void testNegativeSyntheticId()
+    {
+        JdbcColumnHandle column = getDefaultColumnHandleBuilder()
+                .setColumnName("column_0")
+                .build();
+
+        assertThatThrownBy(() -> syntheticColumnHandleBuilder.get(column, -2147483648)).isInstanceOf(VerifyException.class);
+    }
+
+    private static JdbcColumnHandle.Builder getDefaultColumnHandleBuilder()
+    {
+        return JdbcColumnHandle.builder()
+                .setJdbcTypeHandle(JDBC_VARCHAR)
+                .setColumnType(VARCHAR);
+    }
+}
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
@@ -19,6 +19,7 @@
 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;
 
@@ -30,15 +31,17 @@ public class IgniteJdbcMetadataFactory
     private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;
 
     @Inject
-    public IgniteJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
+    public IgniteJdbcMetadataFactory(JdbcClient jdbcClient,
+                Set<JdbcQueryEventListener> jdbcQueryEventListeners,
+                SyntheticColumnHandleBuilder syntheticColumnHandleBuilder)
     {
-        super(jdbcClient, jdbcQueryEventListeners);
+        super(jdbcClient, jdbcQueryEventListeners, syntheticColumnHandleBuilder);
         this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "jdbcQueryEventListeners is null"));
     }
 
     @Override
     protected JdbcMetadata create(JdbcClient transactionCachingJdbcClient)
     {
-        return new IgniteMetadata(transactionCachingJdbcClient, jdbcQueryEventListeners);
+        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
@@ -24,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;
@@ -57,9 +58,11 @@ public class IgniteMetadata
     private final JdbcClient igniteClient;
 
     @Inject
-    public IgniteMetadata(JdbcClient igniteClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
+    public IgniteMetadata(JdbcClient igniteClient,
+                Set<JdbcQueryEventListener> jdbcQueryEventListeners,
+                SyntheticColumnHandleBuilder syntheticColumnHandleBuilder)
     {
-        super(igniteClient, false, jdbcQueryEventListeners);
+        super(igniteClient, false, jdbcQueryEventListeners, syntheticColumnHandleBuilder);
         this.igniteClient = requireNonNull(igniteClient, "igniteClient is null");
     }
 

diff --git a/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOracleConnectorTest.java b/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOracleConnectorTest.java
@@ -17,19 +17,24 @@
 import io.airlift.testing.Closeables;
 import io.trino.testing.QueryRunner;
 import io.trino.testing.sql.SqlExecutor;
+import io.trino.testing.sql.TestTable;
 import org.testng.annotations.AfterClass;
 import org.testng.annotations.Test;
 
+import static io.trino.plugin.jdbc.SyntheticColumnHandleBuilder.DEFAULT_COLUMN_ALIAS_LENGTH;
 import static io.trino.plugin.oracle.TestingOracleServer.TEST_PASS;
 import static io.trino.plugin.oracle.TestingOracleServer.TEST_SCHEMA;
 import static io.trino.plugin.oracle.TestingOracleServer.TEST_USER;
 import static java.lang.String.format;
 import static java.util.stream.Collectors.joining;
 import static java.util.stream.IntStream.range;
+import static org.assertj.core.api.Assertions.assertThat;
 
 public class TestOracleConnectorTest
         extends BaseOracleConnectorTest
 {
+    private static final String MAXIMUM_LENGTH_COLUMN_IDENTIFIER = "z".repeat(DEFAULT_COLUMN_ALIAS_LENGTH);
+
     private TestingOracleServer oracleServer;
 
     @Override
@@ -95,4 +100,16 @@ public void execute(String sql)
             }
         };
     }
+
+    @Test
+    public void testPushdownJoinWithLongNameSucceeds()
+    {
+        try (TestTable table = new TestTable(getQueryRunner()::execute, "long_identifier", "(%s bigint)".formatted(MAXIMUM_LENGTH_COLUMN_IDENTIFIER))) {
+            assertThat(query(joinPushdownEnabled(getSession()), """
+                    SELECT r.name, t.%s, n.name
+                    FROM %s t JOIN region r ON r.regionkey = t.%s
+                    JOIN nation n ON r.regionkey = n.regionkey""".formatted(MAXIMUM_LENGTH_COLUMN_IDENTIFIER, table.getName(), MAXIMUM_LENGTH_COLUMN_IDENTIFIER)))
+                    .isFullyPushedDown();
+        }
+    }
 }
diff --git a/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java b/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClientModule.java
@@ -50,6 +50,7 @@
 import io.trino.plugin.jdbc.QueryBuilder;
 import io.trino.plugin.jdbc.ReusableConnectionFactoryModule;
 import io.trino.plugin.jdbc.StatsCollecting;
+import io.trino.plugin.jdbc.SyntheticColumnHandleBuilderModule;
 import io.trino.plugin.jdbc.TypeHandlingJdbcConfig;
 import io.trino.plugin.jdbc.TypeHandlingJdbcSessionProperties;
 import io.trino.plugin.jdbc.credential.EmptyCredentialProvider;
@@ -150,6 +151,7 @@ protected void setup(Binder binder)
         install(new JdbcDiagnosticModule());
         install(new IdentifierMappingModule());
         install(new DecimalModule());
+        install(new SyntheticColumnHandleBuilderModule());
     }
 
     private void checkConfiguration(String connectionUrl)

diff --git a/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java b/plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMetadata.java
@@ -24,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.AggregateFunction;
 import io.trino.spi.connector.AggregationApplicationResult;
@@ -84,9 +85,12 @@ public class PhoenixMetadata
     private final IdentifierMapping identifierMapping;
 
     @Inject
-    public PhoenixMetadata(PhoenixClient phoenixClient, IdentifierMapping identifierMapping, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
+    public PhoenixMetadata(PhoenixClient phoenixClient,
+            IdentifierMapping identifierMapping,
+            Set<JdbcQueryEventListener> jdbcQueryEventListeners,
+            SyntheticColumnHandleBuilder syntheticColumnHandleBuilder)
     {
-        super(phoenixClient, false, jdbcQueryEventListeners);
+        super(phoenixClient, false, jdbcQueryEventListeners, syntheticColumnHandleBuilder);
         this.phoenixClient = requireNonNull(phoenixClient, "phoenixClient is null");
         this.identifierMapping = requireNonNull(identifierMapping, "identifierMapping is null");
     }