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.

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

Multi-join pushdown appends recursively _<number> to column names so that they exceed maximum alias length #18642

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* 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 io.trino.spi.connector.ConnectorSession;

import static com.google.common.base.Splitter.fixedLength;
import static com.google.common.base.Strings.padStart;

public class ColumnWithAliasFormatter
{
public static final int DEFAULT_COLUMN_ALIAS_LENGTH = 30;
public static final int ORIGINAL_COLUMN_NAME_LENGTH = 24;
Comment on lines +23 to +24
Copy link
Member

Choose a reason for hiding this comment

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

the code lives in trino-base-jdbc but the values are specific to Oracle.

Also consider adding a explanatory comment about why ORIGINAL_COLUMN_NAME_LENGTH = DEFAULT_COLUMN_ALIAS_LENGTH - 6. Where does the magic value 6 come from?

Copy link
Member

Choose a reason for hiding this comment

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

the code lives in trino-base-jdbc but the values are specific to Oracle.

It's ok to ignore for now since as follow-up we'll make this general for all connectors.

Copy link
Member

Choose a reason for hiding this comment

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

Also consider adding a explanatory comment about why ORIGINAL_COLUMN_NAME_LENGTH = DEFAULT_COLUMN_ALIAS_LENGTH - 6. Where does the magic value 6 come from?

This still applies.


public JdbcColumnHandle format(ConnectorSession session, JdbcColumnHandle column, int nextSyntheticColumnId)
Copy link
Member

Choose a reason for hiding this comment

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

ConnectorSession seems unused.

Copy link
Member

Choose a reason for hiding this comment

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

maybe getAliasedColumnHandle?

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe name the class SyntheticColumnHandleProvider/Builder/etc. and the method getSyntheticColumnHandle.

{
int sequentialNumberLength = DEFAULT_COLUMN_ALIAS_LENGTH - ORIGINAL_COLUMN_NAME_LENGTH - 1;

String originalColumnNameTruncated = fixedLength(ORIGINAL_COLUMN_NAME_LENGTH)
.split(column.getColumnName())
.iterator()
.next();
String formatString = "%s_%0" + sequentialNumberLength + "d";
Copy link
Member

Choose a reason for hiding this comment

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

is formatString used?

String columnName = originalColumnNameTruncated + "_" + padStart(Integer.toString(nextSyntheticColumnId), sequentialNumberLength, '0');
Copy link
Member

Choose a reason for hiding this comment

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

why do we pad the next synthetic id part, it leads to identifiers with long numbers in them. Is this required?

Copy link
Member

Choose a reason for hiding this comment

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

Also what happens when the nextSyntheticColumnId is "large" and causes the generated column name to exceed DEFAULT_COLUMN_ALIAS_LENGTH?

return JdbcColumnHandle.builderFrom(column)
.setColumnName(columnName)
.build();
}
}
Original file line number Diff line number Diff line change
@@ -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 ColumnWithAliasFormatterModule
extends AbstractModule
{
@Override
public void configure()
{
bind(ColumnWithAliasFormatter.class).in(Singleton.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 ColumnWithAliasFormatter aliasFormatter;

public DefaultJdbcMetadata(JdbcClient jdbcClient,
boolean precalculateStatisticsForPushdown,
Set<JdbcQueryEventListener> jdbcQueryEventListeners,
ColumnWithAliasFormatter aliasFormatter)
{
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");
}

@Override
Expand Down Expand Up @@ -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, aliasFormatter.format(session, 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, aliasFormatter.format(session, column, nextSyntheticColumnId));
nextSyntheticColumnId++;
}
Map<JdbcColumnHandle, JdbcColumnHandle> newRightColumns = newRightColumnsBuilder.buildOrThrow();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,14 @@ public class DefaultJdbcMetadataFactory
private final JdbcClient jdbcClient;
private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;

protected final ColumnWithAliasFormatter aliasFormatter;

@Inject
public DefaultJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
public DefaultJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners, ColumnWithAliasFormatter aliasFormatter)
{
this.jdbcClient = requireNonNull(jdbcClient, "jdbcClient is null");
this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "queryEventListeners is null"));
this.aliasFormatter = requireNonNull(aliasFormatter, "aliasFormatter is null");
}

@Override
Expand All @@ -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, aliasFormatter);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public void setup(Binder binder)
install(new JdbcDiagnosticModule());
install(new IdentifierMappingModule());
install(new RemoteQueryModifierModule());
install(new ColumnWithAliasFormatterModule());

newOptionalBinder(binder, ConnectorAccessControl.class);
newOptionalBinder(binder, QueryBuilder.class).setDefault().to(DefaultQueryBuilder.class).in(Scopes.SINGLETON);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* 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.Inject;
import io.trino.testing.TestingConnectorSession;
import org.testng.annotations.Guice;
import org.testng.annotations.Test;

import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_VARCHAR;
import static io.trino.spi.type.VarcharType.VARCHAR;
import static org.assertj.core.api.Assertions.assertThat;

@Guice(modules = ColumnWithAliasFormatterModule.class)
public class TestColumnWithAliasFormatter
{
@Inject
private ColumnWithAliasFormatter actor;
Comment on lines +25 to +29
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Guice(modules = ColumnWithAliasFormatterModule.class)
public class TestColumnWithAliasFormatter
{
@Inject
private ColumnWithAliasFormatter actor;
public class TestColumnWithAliasFormatter
{
private final ColumnWithAliasFormatter columnAliasFormatter = new ColumnWithAliasFormatter();

equivalent.


private final TestingConnectorSession session = TestingConnectorSession.builder().build();

@Test
public void testTooLongName()
{
JdbcColumnHandle column = getDefaultColumnHandleBuilder()
.setColumnName("column_with_over_twenty_characters")
.build();

JdbcColumnHandle result = actor.format(session, column, 100);

assertThat(result.getColumnName()).isEqualTo("column_with_over_twenty__00100");
}

@Test
public void testTooShortName()
{
JdbcColumnHandle column = getDefaultColumnHandleBuilder()
.setColumnName("column_0")
.build();

JdbcColumnHandle result = actor.format(session, column, 999);

assertThat(result.getColumnName()).isEqualTo("column_0_00999");
}

private static JdbcColumnHandle.Builder getDefaultColumnHandleBuilder()
{
return JdbcColumnHandle.builder()
.setJdbcTypeHandle(JDBC_VARCHAR)
.setColumnType(VARCHAR);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
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;
Expand All @@ -34,6 +35,7 @@
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;
Expand All @@ -54,25 +56,37 @@
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;

@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(),
aliasFormatter);
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(),
aliasFormatter);
ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());

assertThatThrownBy(() -> {
Expand All @@ -87,7 +101,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(),
aliasFormatter);
ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());

ConnectorSession session = TestingConnectorSession.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

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;
Expand All @@ -30,15 +31,16 @@ public class IgniteJdbcMetadataFactory
private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;

@Inject
public IgniteJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
public IgniteJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners,
ColumnWithAliasFormatter aliasFormatter)
{
super(jdbcClient, jdbcQueryEventListeners);
super(jdbcClient, jdbcQueryEventListeners, aliasFormatter);
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, aliasFormatter);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
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;
Expand Down Expand Up @@ -57,9 +58,10 @@ public class IgniteMetadata
private final JdbcClient igniteClient;

@Inject
public IgniteMetadata(JdbcClient igniteClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
public IgniteMetadata(JdbcClient igniteClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners,
ColumnWithAliasFormatter aliasFormatter)
{
super(igniteClient, false, jdbcQueryEventListeners);
super(igniteClient, false, jdbcQueryEventListeners, aliasFormatter);
this.igniteClient = requireNonNull(igniteClient, "igniteClient is null");
}

Expand Down
Loading