Skip to content

Refactor to always use JdbcColumnHandle.Builder instead of a constructor #18718

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

Closed
dominikzalewski wants to merge 1 commit into from
Closed

Refactor to always use JdbcColumnHandle.Builder instead of a constructor #18718

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
6 changes: 5 additions & 1 deletion 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 @@ -280,7 +280,11 @@ protected List<JdbcColumnHandle> getColumns(ConnectorSession session, Connection
Type type = toColumnMapping(session, connection, jdbcTypeHandle)
.orElseThrow(() -> new UnsupportedOperationException(format("Unsupported type: %s of column: %s", jdbcTypeHandle, name)))
.getType();
columns.add(new JdbcColumnHandle(name, jdbcTypeHandle, type));
columns.add(JdbcColumnHandle.builder()
.setColumnName(name)
.setJdbcTypeHandle(jdbcTypeHandle)
.setColumnType(type)
.build());
}
return columns.build();
}
Expand Down
9 changes: 5 additions & 4 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 @@ -874,10 +874,11 @@ public ColumnHandle getMergeRowIdColumnHandle(ConnectorSession session, Connecto
{
verify(!isTableHandleForProcedure(tableHandle), "Not a table reference: %s", tableHandle);
// The column is used for row-level merge, which is not supported, but it's required during analysis anyway.
return new JdbcColumnHandle(
MERGE_ROW_ID,
new JdbcTypeHandle(Types.BIGINT, Optional.of("bigint"), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()),
BIGINT);
return JdbcColumnHandle.builder()
.setColumnName(MERGE_ROW_ID)
.setJdbcTypeHandle(new JdbcTypeHandle(Types.BIGINT, Optional.of("bigint"), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty()))
.setColumnType(BIGINT)
.build();
}

@Override
Expand Down
9 changes: 9 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcColumnHandle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,15 @@ public long getRetainedSizeInBytes()
+ jdbcTypeHandle.getRetainedSizeInBytes();
}

public static JdbcColumnHandle createJdbcColumnHandle(String columnName, JdbcTypeHandle jdbcTypeHandle, Type columnType)
Copy link
Copy Markdown
Member

@kokosing kokosing Aug 24, 2023

Choose a reason for hiding this comment

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

Refactor to always use JdbcColumnHandle.Builder instead of a constructor

createJdbcColumnHandle method defeats the purpose of this commit. What is the difference between factory method and constructor? I would inline this method.

{
return builder()
.setColumnName(columnName)
.setJdbcTypeHandle(jdbcTypeHandle)
.setColumnType(columnType)
.build();
}

public static Builder builder()
{
return new Builder();
Expand Down
11 changes: 6 additions & 5 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 @@ -42,6 +42,7 @@
import java.util.function.Function;

import static io.airlift.slice.Slices.utf8Slice;
import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
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 @@ -133,9 +134,9 @@ public void testGetColumnHandles()
{
// known table
assertThat(metadata.getColumnHandles(SESSION, tableHandle)).isEqualTo(ImmutableMap.of(
"text", new JdbcColumnHandle("TEXT", JDBC_VARCHAR, VARCHAR),
"text_short", new JdbcColumnHandle("TEXT_SHORT", JDBC_VARCHAR, createVarcharType(32)),
"value", new JdbcColumnHandle("VALUE", JDBC_BIGINT, BIGINT)));
"text", createJdbcColumnHandle("TEXT", JDBC_VARCHAR, VARCHAR),
"text_short", createJdbcColumnHandle("TEXT_SHORT", JDBC_VARCHAR, createVarcharType(32)),
"value", createJdbcColumnHandle("VALUE", JDBC_BIGINT, BIGINT)));

// unknown table
unknownTableColumnHandle(new JdbcTableHandle(new SchemaTableName("unknown", "unknown"), new RemoteTableName(Optional.of("unknown"), Optional.of("unknown"), "unknown"), Optional.empty()));
Expand Down Expand Up @@ -220,7 +221,7 @@ public void testListTables()
@Test
public void getColumnMetadata()
{
assertThat(metadata.getColumnMetadata(SESSION, tableHandle, new JdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR))).isEqualTo(new ColumnMetadata("text", VARCHAR));
assertThat(metadata.getColumnMetadata(SESSION, tableHandle, createJdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR))).isEqualTo(new ColumnMetadata("text", VARCHAR));
}

@Test
Expand All @@ -242,7 +243,7 @@ public void testCreateAndAlterTable()
assertThat(layout.getColumns().get(0)).isEqualTo(new ColumnMetadata("text", VARCHAR));
assertThat(layout.getColumns().get(1)).isEqualTo(new ColumnMetadata("x", VARCHAR));

JdbcColumnHandle columnHandle = new JdbcColumnHandle("x", JDBC_VARCHAR, VARCHAR);
JdbcColumnHandle columnHandle = createJdbcColumnHandle("x", JDBC_VARCHAR, VARCHAR);
metadata.dropColumn(SESSION, handle, columnHandle);
layout = metadata.getTableMetadata(SESSION, handle);
assertThat(layout.getColumns()).hasSize(1);
Expand Down
25 changes: 13 additions & 12 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestDefaultJdbcQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static io.airlift.slice.Slices.utf8Slice;
import static io.airlift.testing.Assertions.assertContains;
import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BIGINT;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BOOLEAN;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_CHAR;
Expand Down Expand Up @@ -119,18 +120,18 @@ public void setup()
CharType charType = CharType.createCharType(0);

columns = ImmutableList.of(
new JdbcColumnHandle("col_0", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("col_1", JDBC_DOUBLE, DOUBLE),
new JdbcColumnHandle("col_2", JDBC_BOOLEAN, BOOLEAN),
new JdbcColumnHandle("col_3", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("col_4", JDBC_DATE, DATE),
new JdbcColumnHandle("col_5", JDBC_TIME, TIME_MILLIS),
new JdbcColumnHandle("col_6", JDBC_TIMESTAMP, TIMESTAMP_MILLIS),
new JdbcColumnHandle("col_7", JDBC_TINYINT, TINYINT),
new JdbcColumnHandle("col_8", JDBC_SMALLINT, SMALLINT),
new JdbcColumnHandle("col_9", JDBC_INTEGER, INTEGER),
new JdbcColumnHandle("col_10", JDBC_REAL, REAL),
new JdbcColumnHandle("col_11", JDBC_CHAR, charType));
createJdbcColumnHandle("col_0", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("col_1", JDBC_DOUBLE, DOUBLE),
createJdbcColumnHandle("col_2", JDBC_BOOLEAN, BOOLEAN),
createJdbcColumnHandle("col_3", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("col_4", JDBC_DATE, DATE),
createJdbcColumnHandle("col_5", JDBC_TIME, TIME_MILLIS),
createJdbcColumnHandle("col_6", JDBC_TIMESTAMP, TIMESTAMP_MILLIS),
createJdbcColumnHandle("col_7", JDBC_TINYINT, TINYINT),
createJdbcColumnHandle("col_8", JDBC_SMALLINT, SMALLINT),
createJdbcColumnHandle("col_9", JDBC_INTEGER, INTEGER),
createJdbcColumnHandle("col_10", JDBC_REAL, REAL),
createJdbcColumnHandle("col_11", JDBC_CHAR, charType));

Connection connection = database.getConnection();
try (PreparedStatement preparedStatement = connection.prepareStatement("create table \"test_table\" (" +
Expand Down
25 changes: 13 additions & 12 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

import java.util.Optional;

import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BIGINT;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_DOUBLE;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_REAL;
Expand Down Expand Up @@ -89,9 +90,9 @@ public void testMetadata()
assertThat(table.get().getRequiredNamedRelation().getRemoteTableName().getTableName()).isEqualTo("NUMBERS");
assertThat(table.get().getRequiredNamedRelation().getSchemaTableName()).isEqualTo(schemaTableName);
assertThat(jdbcClient.getColumns(session, table.orElse(null))).containsExactly(
new JdbcColumnHandle("TEXT", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("TEXT_SHORT", JDBC_VARCHAR, createVarcharType(32)),
new JdbcColumnHandle("VALUE", JDBC_BIGINT, BIGINT));
createJdbcColumnHandle("TEXT", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("TEXT_SHORT", JDBC_VARCHAR, createVarcharType(32)),
createJdbcColumnHandle("VALUE", JDBC_BIGINT, BIGINT));
}

@Test
Expand All @@ -101,8 +102,8 @@ public void testMetadataWithSchemaPattern()
Optional<JdbcTableHandle> table = jdbcClient.getTableHandle(session, schemaTableName);
assertThat(table.isPresent()).withFailMessage("table is missing").isTrue();
assertThat(jdbcClient.getColumns(session, table.get())).containsExactly(
new JdbcColumnHandle("TE_T", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("VA%UE", JDBC_BIGINT, BIGINT));
createJdbcColumnHandle("TE_T", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("VA%UE", JDBC_BIGINT, BIGINT));
}

@Test
Expand All @@ -112,10 +113,10 @@ public void testMetadataWithFloatAndDoubleCol()
Optional<JdbcTableHandle> table = jdbcClient.getTableHandle(session, schemaTableName);
assertThat(table.isPresent()).withFailMessage("table is missing").isTrue();
assertThat(jdbcClient.getColumns(session, table.get())).containsExactly(
new JdbcColumnHandle("COL1", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("COL2", JDBC_DOUBLE, DOUBLE),
new JdbcColumnHandle("COL3", JDBC_DOUBLE, DOUBLE),
new JdbcColumnHandle("COL4", JDBC_REAL, REAL));
createJdbcColumnHandle("COL1", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("COL2", JDBC_DOUBLE, DOUBLE),
createJdbcColumnHandle("COL3", JDBC_DOUBLE, DOUBLE),
createJdbcColumnHandle("COL4", JDBC_REAL, REAL));
}

@Test
Expand All @@ -125,9 +126,9 @@ public void testMetadataWithTimestampCol()
Optional<JdbcTableHandle> table = jdbcClient.getTableHandle(session, schemaTableName);
assertThat(table.isPresent()).withFailMessage("table is missing").isTrue();
assertThat(jdbcClient.getColumns(session, table.get())).containsExactly(
new JdbcColumnHandle("TS_3", JDBC_TIMESTAMP, TIMESTAMP_MILLIS),
new JdbcColumnHandle("TS_6", JDBC_TIMESTAMP, TIMESTAMP_MICROS),
new JdbcColumnHandle("TS_9", JDBC_TIMESTAMP, TIMESTAMP_NANOS));
createJdbcColumnHandle("TS_3", JDBC_TIMESTAMP, TIMESTAMP_MILLIS),
createJdbcColumnHandle("TS_6", JDBC_TIMESTAMP, TIMESTAMP_MICROS),
createJdbcColumnHandle("TS_9", JDBC_TIMESTAMP, TIMESTAMP_NANOS));
}

@Test
Expand Down
17 changes: 9 additions & 8 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcColumnHandle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.util.Optional;

import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
import static io.trino.plugin.jdbc.MetadataUtil.COLUMN_CODEC;
import static io.trino.plugin.jdbc.MetadataUtil.assertJsonRoundTrip;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BIGINT;
Expand Down Expand Up @@ -45,15 +46,15 @@ public void testEquivalence()
{
EquivalenceTester.equivalenceTester()
.addEquivalentGroup(
new JdbcColumnHandle("columnName", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("columnName", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("columnName", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("columnName", JDBC_VARCHAR, VARCHAR))
createJdbcColumnHandle("columnName", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("columnName", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("columnName", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("columnName", JDBC_VARCHAR, VARCHAR))
.addEquivalentGroup(
new JdbcColumnHandle("columnNameX", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("columnNameX", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("columnNameX", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("columnNameX", JDBC_VARCHAR, VARCHAR))
createJdbcColumnHandle("columnNameX", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("columnNameX", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("columnNameX", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("columnNameX", JDBC_VARCHAR, VARCHAR))
.check();
}
}
17 changes: 9 additions & 8 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcRecordSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import static com.google.common.util.concurrent.MoreExecutors.newDirectExecutorService;
import static io.airlift.testing.Closeables.closeAll;
import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BIGINT;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_VARCHAR;
import static io.trino.spi.type.BigintType.BIGINT;
Expand Down Expand Up @@ -73,20 +74,20 @@ public void tearDown()
public void testGetColumnTypes()
{
RecordSet recordSet = createRecordSet(ImmutableList.of(
new JdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR),
new JdbcColumnHandle("text_short", JDBC_VARCHAR, createVarcharType(32)),
new JdbcColumnHandle("value", JDBC_BIGINT, BIGINT)));
createJdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR),
createJdbcColumnHandle("text_short", JDBC_VARCHAR, createVarcharType(32)),
createJdbcColumnHandle("value", JDBC_BIGINT, BIGINT)));
assertThat(recordSet.getColumnTypes()).containsExactly(VARCHAR, createVarcharType(32), BIGINT);

recordSet = createRecordSet(ImmutableList.of(
new JdbcColumnHandle("value", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR)));
createJdbcColumnHandle("value", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR)));
assertThat(recordSet.getColumnTypes()).containsExactly(BIGINT, VARCHAR);

recordSet = createRecordSet(ImmutableList.of(
new JdbcColumnHandle("value", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("value", JDBC_BIGINT, BIGINT),
new JdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR)));
createJdbcColumnHandle("value", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("value", JDBC_BIGINT, BIGINT),
createJdbcColumnHandle("text", JDBC_VARCHAR, VARCHAR)));
assertThat(recordSet.getColumnTypes()).containsExactly(BIGINT, BIGINT, VARCHAR);

recordSet = createRecordSet(ImmutableList.of());
Expand Down
5 changes: 3 additions & 2 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcTableHandle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Optional;
import java.util.OptionalLong;

import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
import static io.trino.plugin.jdbc.MetadataUtil.TABLE_CODEC;
import static io.trino.plugin.jdbc.MetadataUtil.assertJsonRoundTrip;

Expand Down Expand Up @@ -70,7 +71,7 @@ private JdbcTableHandle createQueryBasedHandle()
ImmutableList.of(),
Optional.empty(),
OptionalLong.of(1),
Optional.of(ImmutableList.of(new JdbcColumnHandle("i", type, IntegerType.INTEGER))),
Optional.of(ImmutableList.of(createJdbcColumnHandle("i", type, IntegerType.INTEGER))),
Optional.of(ImmutableSet.of()),
0,
Optional.empty());
Expand All @@ -88,7 +89,7 @@ private JdbcTableHandle createNamedHandle()
ImmutableList.of(),
Optional.empty(),
OptionalLong.of(1),
Optional.of(ImmutableList.of(new JdbcColumnHandle("i", type, IntegerType.INTEGER))),
Optional.of(ImmutableList.of(createJdbcColumnHandle("i", type, IntegerType.INTEGER))),
Optional.of(ImmutableSet.of()),
0,
Optional.empty());
Expand Down
6 changes: 3 additions & 3 deletions ...src/test/java/io/trino/plugin/jdbc/expression/BaseTestRewriteLikeWithCaseSensitivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableList;
import io.trino.matching.Match;
import io.trino.plugin.base.expression.ConnectorExpressionRule;
import io.trino.plugin.jdbc.JdbcColumnHandle;
import io.trino.plugin.jdbc.JdbcTypeHandle;
import io.trino.plugin.jdbc.QueryParameter;
import io.trino.spi.connector.ColumnHandle;
Expand All @@ -31,6 +30,7 @@

import static com.google.common.collect.MoreCollectors.toOptional;
import static io.trino.plugin.jdbc.CaseSensitivity.CASE_SENSITIVE;
import static io.trino.plugin.jdbc.JdbcColumnHandle.createJdbcColumnHandle;
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_BIGINT;
import static io.trino.spi.type.VarcharType.VARCHAR;
import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -50,8 +50,8 @@ protected Optional<ParameterizedExpression> apply(Call expression)
@Override
public Map<String, ColumnHandle> getAssignments()
{
return Map.of("case_insensitive_value", new JdbcColumnHandle("case_insensitive_value", JDBC_BIGINT, VARCHAR),
"case_sensitive_value", new JdbcColumnHandle("case_sensitive_value", new JdbcTypeHandle(Types.VARCHAR, Optional.of("varchar"), Optional.of(10), Optional.empty(), Optional.empty(), Optional.of(CASE_SENSITIVE)), VARCHAR));
return Map.of("case_insensitive_value", createJdbcColumnHandle("case_insensitive_value", JDBC_BIGINT, VARCHAR),
"case_sensitive_value", createJdbcColumnHandle("case_sensitive_value", new JdbcTypeHandle(Types.VARCHAR, Optional.of("varchar"), Optional.of(10), Optional.empty(), Optional.empty(), Optional.of(CASE_SENSITIVE)), VARCHAR));
}

@Override
Expand Down