Skip to content

Remove SyntheticColumnHandleBuilder #19134

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

Merged
kokosing merged 1 commit into from
Sep 25, 2023
Merged

Remove SyntheticColumnHandleBuilder #19134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 25 additions & 9 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.plugin.jdbc;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -82,6 +83,7 @@
import static com.google.common.base.Functions.identity;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Splitter.fixedLength;
import static com.google.common.base.Verify.verify;
import static com.google.common.base.Verify.verifyNotNull;
import static com.google.common.collect.ImmutableList.toImmutableList;
Expand All @@ -105,6 +107,8 @@
public class DefaultJdbcMetadata
implements JdbcMetadata
{
public static final int DEFAULT_COLUMN_ALIAS_LENGTH = 30;

private static final String SYNTHETIC_COLUMN_NAME_PREFIX = "_pfgnrtd_";
private static final String DELETE_ROW_ID = "_trino_artificial_column_handle_for_delete_row_id_";
private static final String MERGE_ROW_ID = "$merge_row_id";
Expand All @@ -115,17 +119,11 @@ public class DefaultJdbcMetadata

private final AtomicReference<Runnable> rollbackAction = new AtomicReference<>();

private final SyntheticColumnHandleBuilder syntheticColumnBuilder;

public DefaultJdbcMetadata(JdbcClient jdbcClient,
boolean precalculateStatisticsForPushdown,
Set<JdbcQueryEventListener> jdbcQueryEventListeners,
SyntheticColumnHandleBuilder syntheticColumnBuilder)
public DefaultJdbcMetadata(JdbcClient jdbcClient, boolean precalculateStatisticsForPushdown, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
{
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
Expand Down Expand Up @@ -465,14 +463,14 @@ public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(

ImmutableMap.Builder<JdbcColumnHandle, JdbcColumnHandle> newLeftColumnsBuilder = ImmutableMap.builder();
for (JdbcColumnHandle column : jdbcClient.getColumns(session, leftHandle)) {
newLeftColumnsBuilder.put(column, syntheticColumnBuilder.get(column, nextSyntheticColumnId));
newLeftColumnsBuilder.put(column, createSyntheticColumn(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, syntheticColumnBuilder.get(column, nextSyntheticColumnId));
newRightColumnsBuilder.put(column, createSyntheticColumn(column, nextSyntheticColumnId));
nextSyntheticColumnId++;
}
Map<JdbcColumnHandle, JdbcColumnHandle> newRightColumns = newRightColumnsBuilder.buildOrThrow();
Expand Down Expand Up @@ -529,6 +527,24 @@ public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(
precalculateStatisticsForPushdown));
}

@VisibleForTesting
static JdbcColumnHandle createSyntheticColumn(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();
}

private static Optional<JdbcColumnHandle> getVariableColumnHandle(Map<String, ColumnHandle> assignments, ConnectorExpression expression)
{
requireNonNull(assignments, "assignments is null");
Expand Down
7 changes: 2 additions & 5 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,11 @@ public class DefaultJdbcMetadataFactory
private final JdbcClient jdbcClient;
private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;

protected final SyntheticColumnHandleBuilder syntheticColumnBuilder;

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

@Override
Expand All @@ -54,6 +51,6 @@ public JdbcMetadata create(JdbcTransactionHandle transaction)

protected JdbcMetadata create(JdbcClient transactionCachingJdbcClient)
{
return new DefaultJdbcMetadata(transactionCachingJdbcClient, true, jdbcQueryEventListeners, syntheticColumnBuilder);
return new DefaultJdbcMetadata(transactionCachingJdbcClient, true, jdbcQueryEventListeners);
}
}
1 change: 0 additions & 1 deletion plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ 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);
Expand Down
39 changes: 0 additions & 39 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/SyntheticColumnHandleBuilder.java
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions ...rino-base-jdbc/src/main/java/io/trino/plugin/jdbc/SyntheticColumnHandleBuilderModule.java
View file
Open in desktop

This file was deleted.

41 changes: 33 additions & 8 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestDefaultJdbcMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.plugin.jdbc;

import com.google.common.base.VerifyException;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -42,6 +43,7 @@
import java.util.function.Function;

import static io.airlift.slice.Slices.utf8Slice;
import static io.trino.plugin.jdbc.DefaultJdbcMetadata.createSyntheticColumn;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BIGINT;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_VARCHAR;
import static io.trino.spi.StandardErrorCode.NOT_FOUND;
Expand All @@ -60,8 +62,6 @@ public class TestDefaultJdbcMetadata
private DefaultJdbcMetadata metadata;
private JdbcTableHandle tableHandle;

private final SyntheticColumnHandleBuilder syntheticColumnHandleBuilder = new SyntheticColumnHandleBuilder();

@BeforeMethod
public void setUp()
throws Exception
Expand All @@ -70,8 +70,7 @@ public void setUp()
metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(),
Optional.empty()),
false,
ImmutableSet.of(),
syntheticColumnHandleBuilder);
ImmutableSet.of());
tableHandle = metadata.getTableHandle(SESSION, new SchemaTableName("example", "numbers"));
}

Expand All @@ -81,8 +80,7 @@ public void testSupportsRetriesValidation()
metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(),
Optional.of(false)),
false,
ImmutableSet.of(),
syntheticColumnHandleBuilder);
ImmutableSet.of());
ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());

assertThatThrownBy(() -> {
Expand All @@ -100,8 +98,7 @@ public void testNonTransactionalInsertValidation()
metadata = new DefaultJdbcMetadata(new GroupingSetsEnabledJdbcClient(database.getJdbcClient(),
Optional.of(true)),
false,
ImmutableSet.of(),
syntheticColumnHandleBuilder);
ImmutableSet.of());
ConnectorTableMetadata tableMetadata = new ConnectorTableMetadata(new SchemaTableName("example", "numbers"), ImmutableList.of());

ConnectorSession session = TestingConnectorSession.builder()
Expand Down Expand Up @@ -400,6 +397,34 @@ public void testMultiGroupKeyPredicatePushdown()
"GROUP BY GROUPING SETS ((\"TEXT\", \"VALUE\"), (\"TEXT\"))");
}

@Test
public void testColumnAliasTruncation()
{
assertThat(createSyntheticColumn(column("column_0"), 999).getColumnName())
.isEqualTo("column_0_999");
assertThat(createSyntheticColumn(column("column_with_over_twenty_characters"), 100).getColumnName())
.isEqualTo("column_with_over_twenty_ch_100");
assertThat(createSyntheticColumn(column("column_with_over_twenty_characters"), Integer.MAX_VALUE).getColumnName())
.isEqualTo("column_with_over_tw_2147483647");
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Sep 25, 2023

Choose a reason for hiding this comment

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

minor: are we using soft assertions to understand which test cases actually fail? (as we replaced separate test with a single one)

Copy link
Copy Markdown
Member Author

@kokosing kokosing Sep 25, 2023

Choose a reason for hiding this comment

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

What is soft assertion?

Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Sep 25, 2023

Choose a reason for hiding this comment

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

https://joel-costigliola.github.io/assertj/core/api/org/assertj/core/api/SoftAssertions.html

They don't fail on the first failure in the test, but try to go through all assertions in the test and only then show a report of what failed.

There's a TestNG equivalent, but I'm not an expert on TestNG ;)

Copy link
Copy Markdown
Member Author

@kokosing kokosing Sep 25, 2023

Choose a reason for hiding this comment

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

Thank you for reference. I think regular assertions are fine. We don't use soft assertions anywhere so far, and I think there is no reason to surprise people here by doing something different than elsewhere.

I also removed using DataProvider. Copying code in tests could be sometimes better over DRY, because code is more readable. Here to be super tester, we could extract each assertion to separate test method, because they are independent. However that would be overkill too due boilerplate code (DataProvider potentially would better in that case?). Putting assertion in single method is kind of middle group. We do that in Trino where we put related assertion to a single method.

TL;DR; it is all about the taste and habits

}

@Test
public void testNegativeSyntheticId()
{
JdbcColumnHandle column = column("column_0");

assertThatThrownBy(() -> createSyntheticColumn(column, -2147483648)).isInstanceOf(VerifyException.class);
}

private static JdbcColumnHandle column(String columnName)
{
return JdbcColumnHandle.builder()
.setJdbcTypeHandle(JDBC_VARCHAR)
.setColumnType(VARCHAR)
.setColumnName(columnName)
.build();
}

private JdbcTableHandle applyCountAggregation(ConnectorSession session, ConnectorTableHandle tableHandle, List<List<ColumnHandle>> groupByColumns)
{
Optional<AggregationApplicationResult<ConnectorTableHandle>> aggResult = metadata.applyAggregation(
Expand Down
68 changes: 0 additions & 68 deletions .../trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestSyntheticColumnHandleBuilder.java
View file
Open in desktop

This file was deleted.

9 changes: 3 additions & 6 deletions plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteJdbcMetadataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
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;

Expand All @@ -31,17 +30,15 @@ public class IgniteJdbcMetadataFactory
private final Set<JdbcQueryEventListener> jdbcQueryEventListeners;

@Inject
public IgniteJdbcMetadataFactory(JdbcClient jdbcClient,
Set<JdbcQueryEventListener> jdbcQueryEventListeners,
SyntheticColumnHandleBuilder syntheticColumnHandleBuilder)
public IgniteJdbcMetadataFactory(JdbcClient jdbcClient, Set<JdbcQueryEventListener> jdbcQueryEventListeners)
{
super(jdbcClient, jdbcQueryEventListeners, syntheticColumnHandleBuilder);
super(jdbcClient, jdbcQueryEventListeners);
this.jdbcQueryEventListeners = ImmutableSet.copyOf(requireNonNull(jdbcQueryEventListeners, "jdbcQueryEventListeners is null"));
}

@Override
protected JdbcMetadata create(JdbcClient transactionCachingJdbcClient)
{
return new IgniteMetadata(transactionCachingJdbcClient, jdbcQueryEventListeners, syntheticColumnBuilder);
return new IgniteMetadata(transactionCachingJdbcClient, jdbcQueryEventListeners);
}
}
7 changes: 2 additions & 5 deletions plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgniteMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
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;
Expand Down Expand Up @@ -58,11 +57,9 @@ public class IgniteMetadata
private final JdbcClient igniteClient;

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

Expand Down
Loading