Skip to content

Allow different synthetic column name length limits in JDBC connectors #18984

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
hashhar merged 6 commits into from
Nov 28, 2023
Merged

Allow different synthetic column name length limits in JDBC connectors #18984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
192e5b4
Rename method to clarify scope
hashhar Oct 10, 2023
2062448
Verify aggregation pushdown isn't affected by column name length limits
hashhar Sep 10, 2023
598ead5
Introduce JdbcClient#getMaxColumnNameLength
hashhar Sep 10, 2023
8d093ec
Allow having different column length limits per connector
hashhar Oct 11, 2023
7a76821
Handle edge cases in createSyntheticJoinProjectionColumn
hashhar Oct 11, 2023
9d3fe7e
Apply generated column name truncation to all affected connectors
hashhar Oct 13, 2023
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
21 changes: 21 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ public abstract class BaseJdbcClient
private final IdentifierMapping identifierMapping;
private final boolean supportsRetries;
private final JdbcRemoteIdentifiersFactory jdbcRemoteIdentifiersFactory = new JdbcRemoteIdentifiersFactory(this);
private Integer maxColumnNameLength;

public BaseJdbcClient(
String identifierQuote,
Expand Down Expand Up @@ -1435,6 +1436,26 @@ public OptionalInt getMaxWriteParallelism(ConnectorSession session)
return OptionalInt.of(getWriteParallelism(session));
}

protected OptionalInt getMaxColumnNameLengthFromDatabaseMetaData(ConnectorSession session)
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

Sorry for 'late notice' and sending this after all the other comments. It only occurred to me to ask this question after I detached myself from the keyboard.

I presume you are not enabling this for all the databases due to 'security reasons', in the sense you want to limit the impact of this change in case something goes wrong?

Is the target state then to have the same call to this method in every Client?

It really feels like what we should do here is detach the code deployment from the rollout (for instance via a feature toggle).

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

here's the process I followed:

  • add a test which exposes the problem
  • add a mechanism to customise the lengths for each database since the tests showed there's no single value for all of them
  • re-run the test with the default lenght being unlimited
  • override this method with implementation only in databases where there was a failure

at this point we know (via tests) that the ones without the override do actually support identifiers longer than 65k characters.
we also know that by enabling this path by default for all connectors would incur I/O even in connectors where there's no limit. i.e. we'll ask the DB what the limit is - it'll say no limit. We already know this to be true statically so why incur the I/O and slow down query planning and analysis.

{
if (maxColumnNameLength != null) {
// According to JavaDoc of DatabaseMetaData#getMaxColumnNameLength a value of 0 signifies that the limit is unknown
if (maxColumnNameLength == 0) {
return OptionalInt.empty();
}

return OptionalInt.of(maxColumnNameLength);
}

try (Connection connection = connectionFactory.openConnection(session)) {
maxColumnNameLength = connection.getMetaData().getMaxColumnNameLength();
return OptionalInt.of(maxColumnNameLength);
}
catch (SQLException e) {
throw new TrinoException(JDBC_ERROR, e);
}
}

protected void verifySchemaName(DatabaseMetaData databaseMetadata, String schemaName)
throws SQLException
{
Expand Down
6 changes: 6 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,12 @@ public OptionalInt getMaxWriteParallelism(ConnectorSession session)
return delegate.getMaxWriteParallelism(session);
}

@Override
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

I'm not sure whether to me it makes it more readable that this change is introduced in a separate commit. According to the change in 'commit splitting strategy' we were proposing with @kokosing the other day I'm trying to ask the question: If any other change from this PR has to be reverted, do I want this one to stay?

To me it feels like it should be squashed with the next commit that actually makes use of that change.

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

it helps make the next commit's diff much cleaner - no need to pay attention to the interface change when you can just focus on the implementation details which are the ones which need to be actually reviewed.

As before, there are no "hard rules, only guidelines". In this case practically people can not revert this commit without any impact since it's a no-op so it doesn't matter.

public OptionalInt getMaxColumnNameLength(ConnectorSession session)
Comment thread
hashhar marked this conversation as resolved.
{
return delegate.getMaxColumnNameLength(session);
}

public void onDataChanged(SchemaTableName table)
{
invalidateAllIf(statisticsCache, key -> key.mayReference(table));
Expand Down
62 changes: 44 additions & 18 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 @@ -107,8 +107,6 @@
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 Down Expand Up @@ -391,19 +389,13 @@ public Optional<AggregationApplicationResult<ConnectorTableHandle>> applyAggrega
return Optional.empty();
Comment thread
hashhar marked this conversation as resolved.
}

String columnName = SYNTHETIC_COLUMN_NAME_PREFIX + nextSyntheticColumnId;
JdbcColumnHandle newColumn = createSyntheticAggregationColumn(aggregate, expression.get().getJdbcTypeHandle(), nextSyntheticColumnId);
nextSyntheticColumnId++;
JdbcColumnHandle newColumn = JdbcColumnHandle.builder()
.setColumnName(columnName)
.setJdbcTypeHandle(expression.get().getJdbcTypeHandle())
.setColumnType(aggregate.getOutputType())
.setComment(Optional.of("synthetic"))
.build();

newColumns.add(newColumn);
projections.add(new Variable(newColumn.getColumnName(), aggregate.getOutputType()));
resultAssignments.add(new Assignment(newColumn.getColumnName(), newColumn, aggregate.getOutputType()));
expressions.put(columnName, new ParameterizedExpression(expression.get().getExpression(), expression.get().getParameters()));
expressions.put(newColumn.getColumnName(), new ParameterizedExpression(expression.get().getExpression(), expression.get().getParameters()));
}

List<JdbcColumnHandle> newColumnsList = newColumns.build();
Expand Down Expand Up @@ -431,6 +423,19 @@ public Optional<AggregationApplicationResult<ConnectorTableHandle>> applyAggrega
return Optional.of(new AggregationApplicationResult<>(handle, projections.build(), resultAssignments.build(), ImmutableMap.of(), precalculateStatisticsForPushdown));
}

@VisibleForTesting
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

So the other day I had a discussion with @kokosing about the change I wanted to do to get rid of the problem that if we introduce a dependent object to this hierarchy, we wind up with the 'hell' in plugins and backporting that we don't want to come back to. @kokosing was arguing that I should not be making that change upfront claiming the YAGNI principle. Now, to be on the 'good advisory' side, I have to ask this question, probably not to you @hashhar , but to @kokosing. Does this instance of introducing the 'smell' of '@VisibleForTesting' yet again justify making the modification we were discussing last time (replace inheritance with composition).

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

see #18984 (comment)

for base-jdbc we've settled on inheritance since it makes implementing new connectors based on it super duper easy.

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

also if i remove the "unit test" and convert to an integration test there won't be this VisibleForTesting needed.

Since the integration test that I came up with (a query with too many aggregation functions so that new aliases get generated) is very "wordy" and also prone to breaking due to planner changes I opted for the unit test + VisibleForTesting.

static JdbcColumnHandle createSyntheticAggregationColumn(AggregateFunction aggregate, JdbcTypeHandle typeHandle, int nextSyntheticColumnId)
Comment thread
hashhar marked this conversation as resolved.
{
// the new column can be max len(SYNTHETIC_COLUMN_NAME_PREFIX) + len(Integer.MAX_VALUE) = 9 + 10 = 19 characters which is small enough to be supported by all databases
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

do we need that comment if a variation of it is already put into the test code?

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

i think it's useful since when reading the source people don't have the test class open side by side.

String columnName = SYNTHETIC_COLUMN_NAME_PREFIX + nextSyntheticColumnId;
return JdbcColumnHandle.builder()
.setColumnName(columnName)
.setJdbcTypeHandle(typeHandle)
.setColumnType(aggregate.getOutputType())
.setComment(Optional.of("synthetic"))
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

I was wondering why and 'for what' do we use the comment field. Does it help in any way that we'll put 'synthetic' word in the comment?

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

for synthetic columns the comment field would only appear in logs and explain plans. so it's useful to identify those in a plan. no other significance.

.build();
}

@Override
public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(
ConnectorSession session,
Expand Down Expand Up @@ -459,15 +464,16 @@ public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(
int nextSyntheticColumnId = max(leftHandle.getNextSyntheticColumnId(), rightHandle.getNextSyntheticColumnId());
Comment thread
hashhar marked this conversation as resolved.

ImmutableMap.Builder<JdbcColumnHandle, JdbcColumnHandle> newLeftColumnsBuilder = ImmutableMap.builder();
OptionalInt maxColumnNameLength = jdbcClient.getMaxColumnNameLength(session);
Comment thread
hashhar marked this conversation as resolved.
for (JdbcColumnHandle column : jdbcClient.getColumns(session, leftHandle)) {
newLeftColumnsBuilder.put(column, createSyntheticColumn(column, nextSyntheticColumnId));
newLeftColumnsBuilder.put(column, createSyntheticJoinProjectionColumn(column, nextSyntheticColumnId, maxColumnNameLength));
nextSyntheticColumnId++;
}
Map<JdbcColumnHandle, JdbcColumnHandle> newLeftColumns = newLeftColumnsBuilder.buildOrThrow();

ImmutableMap.Builder<JdbcColumnHandle, JdbcColumnHandle> newRightColumnsBuilder = ImmutableMap.builder();
for (JdbcColumnHandle column : jdbcClient.getColumns(session, rightHandle)) {
newRightColumnsBuilder.put(column, createSyntheticColumn(column, nextSyntheticColumnId));
newRightColumnsBuilder.put(column, createSyntheticJoinProjectionColumn(column, nextSyntheticColumnId, maxColumnNameLength));
nextSyntheticColumnId++;
}
Map<JdbcColumnHandle, JdbcColumnHandle> newRightColumns = newRightColumnsBuilder.buildOrThrow();
Expand Down Expand Up @@ -525,20 +531,40 @@ public Optional<JoinApplicationResult<ConnectorTableHandle>> applyJoin(
}

@VisibleForTesting
static JdbcColumnHandle createSyntheticColumn(JdbcColumnHandle column, int nextSyntheticColumnId)
static JdbcColumnHandle createSyntheticJoinProjectionColumn(JdbcColumnHandle column, int nextSyntheticColumnId, OptionalInt optionalMaxColumnNameLength)
{
verify(nextSyntheticColumnId >= 0, "nextSyntheticColumnId rolled over and is not monotonically increasing any more");
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

I was wondering, looking at the code you introduced, whether you had a chance in the past to take a look at this presentation by Stuart Marks, who is the person who 'invented' Optionals? The specific fragment of his presentation I'm referring to starts here: https://youtu.be/fBYhtvY19xA?feature=shared&t=611

Perhaps, following his advise there is a way to use orElse here? It looks like both lines with the 'return' statement have a similar columnName construction.

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

orElse / orElseGet can only return an object of type T for Optional<T> which is not this case.

There is ifPresent / ifPresentOrElse which leads to not more readable code because of a very large lambda + atomicreference needing to be used since lambdas need assignments to be effectively final:

    @VisibleForTesting
    static JdbcColumnHandle createSyntheticJoinProjectionColumn(JdbcColumnHandle column, int nextSyntheticColumnId, OptionalInt optionalMaxColumnNameLength)
    {
        verify(nextSyntheticColumnId >= 0, "nextSyntheticColumnId rolled over and is not monotonically increasing any more");

        final String separator = "_";
        AtomicReference<JdbcColumnHandle> ret = new AtomicReference<>();
        optionalMaxColumnNameLength.ifPresentOrElse(maxColumnNameLength -> {
                    int nextSyntheticColumnIdLength = String.valueOf(nextSyntheticColumnId).length();
                    verify(maxColumnNameLength >= nextSyntheticColumnIdLength, "Maximum allowed column name length is %s but next synthetic id has length %s", maxColumnNameLength, nextSyntheticColumnIdLength);

                    if (nextSyntheticColumnIdLength == maxColumnNameLength) {
                        ret.set(JdbcColumnHandle.builderFrom(column)
                                .setColumnName(String.valueOf(nextSyntheticColumnId))
                                .build());
                    }

                    if (nextSyntheticColumnIdLength + separator.length() == maxColumnNameLength) {
                        ret.set(JdbcColumnHandle.builderFrom(column)
                                .setColumnName(separator + nextSyntheticColumnId)
                                .build());
                    }

                    // fixedLength only accepts values > 0, so the cases where the value would be <= 0 are handled above explicitly
                    String truncatedColumnName = fixedLength(maxColumnNameLength - separator.length() - nextSyntheticColumnIdLength)
                            .split(column.getColumnName())
                            .iterator()
                            .next();
                    ret.set(JdbcColumnHandle.builderFrom(column)
                            .setColumnName(truncatedColumnName + separator + nextSyntheticColumnId)
                            .build());
                },
                () -> JdbcColumnHandle.builderFrom(column)
                        .setColumnName(column.getColumnName() + separator + nextSyntheticColumnId)
                        .build());
        return ret.get();
    }


int sequentialNumberLength = String.valueOf(nextSyntheticColumnId).length();
int originalColumnNameLength = DEFAULT_COLUMN_ALIAS_LENGTH - sequentialNumberLength - "_".length();
final String separator = "_";
if (optionalMaxColumnNameLength.isEmpty()) {
return JdbcColumnHandle.builderFrom(column)
.setColumnName(column.getColumnName() + separator + nextSyntheticColumnId)
.build();
}

int maxColumnNameLength = optionalMaxColumnNameLength.getAsInt();
int nextSyntheticColumnIdLength = String.valueOf(nextSyntheticColumnId).length();
verify(maxColumnNameLength >= nextSyntheticColumnIdLength, "Maximum allowed column name length is %s but next synthetic id has length %s", maxColumnNameLength, nextSyntheticColumnIdLength);
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

do we need to go that far to checking that condition? The previous bug report actually proved that for instance for Oracle, it was the DB engine complaining if the alias was too long. So the user experience was that there was a message from a DB engine.

Does it improve that user experience in any way that we will send this 'crypthic' message to the user? Would he/she have a better understanding of the situation if we had that code instead of not having that code at all and relying on the DB engine error reporting?

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

So the user experience was that there was a message from a DB engine.

This does not work in practice. For example for Postgres and Redshift the identifier is silently truncated with no useful error shown to the user and just a generic query failure about "ambiguous columns".

Also Trino is meant to be a uniform interface over your data-sources so where we can we try to provide that uniformity. e.g. CAST('abc' AS varchar(1)) behaves differently in MySQL vs Postgres but Trino will always return same result regardless of what the underlying system is. Same goes for other user-visible aspects.


if (nextSyntheticColumnIdLength == maxColumnNameLength) {
return JdbcColumnHandle.builderFrom(column)
.setColumnName(String.valueOf(nextSyntheticColumnId))
.build();
}

String columnNameTruncated = fixedLength(originalColumnNameLength)
if (nextSyntheticColumnIdLength + separator.length() == maxColumnNameLength) {
return JdbcColumnHandle.builderFrom(column)
.setColumnName(separator + nextSyntheticColumnId)
.build();
}

// fixedLength only accepts values > 0, so the cases where the value would be <= 0 are handled above explicitly
String truncatedColumnName = fixedLength(maxColumnNameLength - separator.length() - nextSyntheticColumnIdLength)
.split(column.getColumnName())
.iterator()
.next();
String columnName = columnNameTruncated + "_" + nextSyntheticColumnId;
return JdbcColumnHandle.builderFrom(column)
.setColumnName(columnName)
.setColumnName(truncatedColumnName + separator + nextSyntheticColumnId)
.build();
}

Expand Down
6 changes: 6 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,4 +450,10 @@ public OptionalInt getMaxWriteParallelism(ConnectorSession session)
{
return delegate().getMaxWriteParallelism(session);
}

@Override
public OptionalInt getMaxColumnNameLength(ConnectorSession session)
{
return delegate().getMaxColumnNameLength(session);
}
}
5 changes: 5 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,9 @@ default Optional<TableScanRedirectApplicationResult> getTableScanRedirection(Con
OptionalLong update(ConnectorSession session, JdbcTableHandle handle);

OptionalInt getMaxWriteParallelism(ConnectorSession session);

default OptionalInt getMaxColumnNameLength(ConnectorSession session)
Copy link
Copy Markdown
Member

@dominikzalewski dominikzalewski Oct 17, 2023

Choose a reason for hiding this comment

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

I'm just thinking out loud whether this is a good place to introduce this method. Currently JdbcClient has methods like 'list tables', 'list schemas' (which is metadata in some way), but also methods like 'create', 'delete' some 'object'. It really feels like this method is not about 'another metadata', but it's a property of the database engine rather than the data itself.

I started asking myself questions like, if it turns out that the 'get' method to query the database for the limit does not return a proper value and we want to kind of hard code it in a 'we know better' way, then how would we do that say for Oracle 11g that has 30 chars limit and Oracle 12 which has 128 limit. Would we be putting a buch of IF statements into this implementation?

I'm not sure whether it's worth introducing a different 'type' of object to handle database restrictions, but it really feels like we are mixing domains here by introducing this change.

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 27, 2023

Choose a reason for hiding this comment

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

Would we be putting a buch of IF statements into this implementation?

Yes, we would. And note that limits aren't the only case where this happens. e.g. rename column syntax differs across MySql versions so MySqlClient#renameColumn has such an impl.

JdbcClient is the "API" for the database. Anything we want to do to remote database is defined here.

{
return OptionalInt.empty();
}
}
6 changes: 6 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,4 +470,10 @@ public OptionalInt getMaxWriteParallelism(ConnectorSession session)
{
return delegate().getMaxWriteParallelism(session);
}

@Override
public OptionalInt getMaxColumnNameLength(ConnectorSession session)
{
return delegate().getMaxColumnNameLength(session);
}
}
20 changes: 20 additions & 0 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2041,6 +2041,26 @@ protected TestTable simpleTable()
return new TestTable(onRemoteDatabase(), format("%s.simple_table", getSession().getSchema().orElseThrow()), "(col BIGINT)", ImmutableList.of("1", "2"));
}

@Test
public void testJoinPushdownWithLongIdentifiers()
{
skipTestUnless(hasBehavior(SUPPORTS_CREATE_TABLE) && hasBehavior(SUPPORTS_JOIN_PUSHDOWN));

String baseColumnName = "col";
int maxLength = maxColumnNameLength()
// Assume 2^16 is enough for most use cases. Add a bit more to ensure 2^16 isn't actual limit.
.orElse(65536 + 5);

String validColumnName = baseColumnName + "z".repeat(maxLength - baseColumnName.length());
try (TestTable left = new TestTable(getQueryRunner()::execute, "test_long_id_l", format("(%s BIGINT)", validColumnName));
TestTable right = new TestTable(getQueryRunner()::execute, "test_long_id_r", format("(%s BIGINT)", validColumnName))) {
assertThat(query(joinPushdownEnabled(getSession()), """
SELECT l.%1$s, r.%1$s
FROM %2$s l JOIN %3$s r ON l.%1$s = r.%1$s""".formatted(validColumnName, left.getName(), right.getName())))
.isFullyPushedDown();
}
}

@Test
public void testDynamicFiltering()
{
Expand Down
47 changes: 38 additions & 9 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 @@ -41,10 +41,12 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalInt;
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.DefaultJdbcMetadata.createSyntheticAggregationColumn;
import static io.trino.plugin.jdbc.DefaultJdbcMetadata.createSyntheticJoinProjectionColumn;
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 Down Expand Up @@ -402,20 +404,47 @@ public void testMultiGroupKeyPredicatePushdown()
@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");
OptionalInt maxLength = OptionalInt.of(30);
assertThat(createSyntheticJoinProjectionColumn(column("no_truncation"), 123, maxLength).getColumnName())
.isEqualTo("no_truncation_123");
assertThat(createSyntheticJoinProjectionColumn(column("long_column_name_gets_truncated"), 123, maxLength).getColumnName())
.isEqualTo("long_column_name_gets_trun_123");
assertThat(createSyntheticJoinProjectionColumn(column("long_id_causes_truncation"), Integer.MAX_VALUE, maxLength).getColumnName())
.isEqualTo("long_id_causes_trun_2147483647");

assertThat(createSyntheticJoinProjectionColumn(column("id_equals_max_length"), 1234, OptionalInt.of(4)).getColumnName())
.isEqualTo("1234");
assertThat(createSyntheticJoinProjectionColumn(column("id_and_separator_equals_max_length"), 1234, OptionalInt.of(5)).getColumnName())
.isEqualTo("_1234");
}

@Test
public void testSyntheticIdExceedsLength()
{
assertThatThrownBy(() -> createSyntheticJoinProjectionColumn(column("id_exceeds_max_length"), 1234, OptionalInt.of(3)))
.isInstanceOf(VerifyException.class)
.hasMessage("Maximum allowed column name length is 3 but next synthetic id has length 4");
}

@Test
public void testNegativeSyntheticId()
{
JdbcColumnHandle column = column("column_0");
//noinspection NumericOverflow
assertThatThrownBy(() -> createSyntheticJoinProjectionColumn(column("negative_id"), Integer.MAX_VALUE + 1, OptionalInt.of(30)))
.isInstanceOf(VerifyException.class)
.hasMessage("nextSyntheticColumnId rolled over and is not monotonically increasing any more");
}

assertThatThrownBy(() -> createSyntheticColumn(column, -2147483648)).isInstanceOf(VerifyException.class);
@Test
public void testAggregationColumnAliasMaxLength()
{
// this test is to ensure that the generated names for aggregation pushdown are short enough to be supported by all databases.
// Oracle has the smallest limit at 30 so any value smaller than that is acceptable.
assertThat(createSyntheticAggregationColumn(
new AggregateFunction("count", BIGINT, List.of(), List.of(), false, Optional.empty()),
JDBC_BIGINT,
Integer.MAX_VALUE).getColumnName().length())
.isEqualTo(19);
Comment thread
hashhar marked this conversation as resolved.
}

private static JdbcColumnHandle column(String columnName)
Expand Down
7 changes: 7 additions & 0 deletions plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
import java.util.function.BiFunction;

Expand Down Expand Up @@ -348,6 +349,12 @@ public void renameSchema(ConnectorSession session, String schemaName, String new
throw new TrinoException(NOT_SUPPORTED, "This connector does not support renaming schemas");
}

@Override
public OptionalInt getMaxColumnNameLength(ConnectorSession session)
{
return getMaxColumnNameLengthFromDatabaseMetaData(session);
}

@Override
public Optional<String> getTableComment(ResultSet resultSet)
throws SQLException
Expand Down
17 changes: 0 additions & 17 deletions plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOracleConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,22 @@
import io.airlift.testing.Closeables;
import io.trino.testing.QueryRunner;
import io.trino.testing.sql.SqlExecutor;
import io.trino.testing.sql.TestTable;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;

import static io.trino.plugin.jdbc.DefaultJdbcMetadata.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;
import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;

@TestInstance(PER_CLASS)
public class TestOracleConnectorTest
extends BaseOracleConnectorTest
{
private static final String MAXIMUM_LENGTH_COLUMN_IDENTIFIER = "z".repeat(DEFAULT_COLUMN_ALIAS_LENGTH);

private TestingOracleServer oracleServer;

@Override
Expand Down Expand Up @@ -103,16 +98,4 @@ 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();
}
}
}
7 changes: 7 additions & 0 deletions plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.OptionalLong;
import java.util.UUID;
import java.util.function.BiFunction;
Expand Down Expand Up @@ -918,6 +919,12 @@ public OptionalLong update(ConnectorSession session, JdbcTableHandle handle)
}
}

@Override
public OptionalInt getMaxColumnNameLength(ConnectorSession session)
{
return getMaxColumnNameLengthFromDatabaseMetaData(session);
}

@Override
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle)
{
Expand Down
Loading