Support column properties with jdbc connector #25174

hqbhoho · 2025-02-27T01:36:11Z

Description

Add column properties for base jdbc connector to support auto_increment, default value and other column attribute.
When running SHOW CREATE TABLE include column properties present for these column attribute.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

## Mysql
*  Add support for creating tables with `auto_increment` column property.

github-actions · 2025-03-20T17:03:04Z

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

hqbhoho · 2025-03-27T03:58:37Z

The JdbcColumnHandle may include all the information returned by DatabaseMetaData.getColumns()?
If we do not add additional fields to the JdbcColumnHandle, due to serialization constraints, it seems that we can only implement a JdbcExtraColumnHandle in the base-jdbc package and configure JsonTypeInfo like this:

@JsonTypeInfo(
        use = JsonTypeInfo.Id.NAME,
        property = "@type")
@JsonSubTypes({
        @JsonSubTypes.Type(value = JdbcExtraColumnHandle.class, name = "system:io.trino.plugin.jdbc.JdbcExtraColumnHandle"),
        @JsonSubTypes.Type(value = JdbcColumnHandle.class, name = "system:io.trino.plugin.jdbc.JdbcColumnHandle")
})
public class JdbcColumnHandle
        implements ColumnHandle
{
}

public class JdbcExtraColumnHandle
        extends JdbcColumnHandle
{
    private final boolean autoIncrement;
    ...
}

@chenjian2664 @ebyhr So which way is better?

ebyhr

Please update MySQL connector's documentation.

ebyhr · 2025-03-31T02:35:38Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnector.java

@@ -81,6 +83,9 @@ public JdbcConnector(
        this.tableProperties = tableProperties.stream()
                .flatMap(tablePropertiesProvider -> tablePropertiesProvider.getTableProperties().stream())
                .collect(toImmutableList());
+        this.columnProperties = columnProperties.stream()


Add requireNonNull.

ebyhr · 2025-03-31T02:39:31Z

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

+        boolean isAutoIncrement = (boolean) column.getProperties().getOrDefault(AUTO_INCREMENT, false);
+        if (isAutoIncrement) {
+            Type type = column.getType();
+            if (!(type instanceof BigintType) && !(type instanceof IntegerType)) {


Replace order of IntegerType and BigintType. We usually order as smaller → larger.
Also, I think this limitation is wrong. We can enable auto increment on tinyint and smallint types.

Replace order of IntegerType and BigintType. We usually order as smaller → larger. Also, I think this limitation is wrong. We can enable auto increment on tinyint and smallint types.

Using AUTO_INCREMENT with float and double columns is deprecated as of MySQL 8.0.17 and support for it to be removed in a future version of MySQL. So we should only support integer types. I initially thought that the value ranges of TINYINT and SMALLINT were limited, so I assumed these two data types were not supported at first. I have added them.

ebyhr · 2025-03-31T02:46:36Z

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlColumnProperties.java

+        columnProperties = ImmutableList.<PropertyMetadata<?>>builder()
+                .add(booleanProperty(
+                        AUTO_INCREMENT,
+                        "If column can auto increment",


Suggested change

"If column can auto increment",

"Generate a unique identity for new rows",

ebyhr · 2025-03-31T02:47:31Z

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlMetadata.java

+                .collect(toImmutableList());
+    }
+
+    public static ColumnMetadata getColumnMetadata(JdbcColumnHandle handle)


This method can be private.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

ebyhr · 2025-03-31T02:48:32Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

+
+        verifyCreateTableDefinition(
+                tablePrefix,
+                "(a bigint NOT NULL with (auto_increment = true), b bigint, c bigint) with (primary_key = ARRAY['a'])",


Uppercase with. Same for others.

ebyhr · 2025-04-01T04:48:22Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcColumnHandle.java

@@ -30,7 +30,7 @@
 import static io.airlift.slice.SizeOf.sizeOf;
 import static java.util.Objects.requireNonNull;

-public class JdbcColumnHandle
+public final class JdbcColumnHandle


The 1st commit added removed final, and the 2nd commit restored it. Please completely remove the change.

ebyhr · 2025-04-01T04:48:33Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcColumnHandle.java

@@ -162,13 +172,14 @@ public static Builder builderFrom(JdbcColumnHandle handle)
        return new Builder(handle);
    }

-    public static class Builder
+    public static final class Builder


The 1st commit added removed final, and the 2nd commit restored it. Please completely remove the change.

ebyhr · 2025-04-01T04:51:31Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

+        assertQueryFails("CREATE TABLE " + tableName + " (a bigint, b bigint, c bigint) WITH (primary_key = ARRAY['a'])",
                "Primary key must be NOT NULL in MySQL");
        // primary key must exist in column list
-        assertQueryFails("CREATE TABLE " + tableName + " (a bigint, b bigint, c bigint) with (primary_key = ARRAY['d'])",
+        assertQueryFails("CREATE TABLE " + tableName + " (a bigint, b bigint, c bigint) WITH (primary_key = ARRAY['d'])",
                "Column 'd' specified in property 'primary_key' doesn't exist in table");
-        assertQueryFails("CREATE TABLE " + tableName + " (a bigint, b bigint, c bigint) with (primary_key = ARRAY['A'])",
+        assertQueryFails("CREATE TABLE " + tableName + " (a bigint, b bigint, c bigint) WITH (primary_key = ARRAY['A'])",


This change is unrelated to "Add support create table with auto increment column". Please separate a commit.

ebyhr · 2025-04-01T04:53:27Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

+        }
+    }
+
+    private void verifyCreateTableDefinition(String tablePrefix, String tableDefinition, String showCreateTableFormat)


Why do we need to take a tablePrefix parameter from callers?

It‘s not necessary. I have remove it.

ebyhr · 2025-04-01T04:53:43Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

+    {
+        try (TestTable table = newTrinoTable("test_insert_with_auto_increment_column_", "(a bigint NOT NULL WITH (auto_increment = true), b bigint) WITH (primary_key = ARRAY['a'])")) {
+            assertUpdate("INSERT INTO " + table.getName() + " (b) VALUES (1), (2)", 2);
+            assertQuery("SELECT a, b FROM " + table.getName(), "VALUES (1, 1), (2, 2)");


Please replace assertQuery with assertThat(query()).

ebyhr · 2025-04-01T04:54:42Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

+    @Test
+    public void testAddColumnWithColumnProperties()
+    {
+        assertQueryFails("ALTER TABLE nation ADD COLUMN test_add_col_properties int WITH (auto_increment=true) ", "This connector does not support adding columns with column properties");


I would avoid modifying TPCH tables, even for negative test cases.

ebyhr · 2025-04-01T04:55:34Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java

+    @Test
+    public void testShowColumnsWithAutoIncrementColumn()
+    {
+        try (TestTable table = newTrinoTable("test_show_columns_with_invalid_auto_increment_", "(a bigint NOT NULL WITH (auto_increment = true), b bigint) WITH (primary_key = ARRAY['a'])")) {


test_show_columns_with_invalid_auto_increment_

What is "invalid" in this test?

No, I am careless!

cla-bot bot added the cla-signed label Feb 27, 2025

hqbhoho marked this pull request as draft February 27, 2025 01:36

github-actions bot added the mysql MySQL connector label Feb 27, 2025

hqbhoho changed the title ~~Feature/support column properties with jdbc connector~~ Support column properties with jdbc connector Feb 27, 2025

github-actions bot added the stale label Mar 20, 2025

hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch 9 times, most recently from 7309388 to dbc00bb Compare March 26, 2025 14:02

hqbhoho marked this pull request as ready for review March 26, 2025 15:08

hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from dbc00bb to 7630693 Compare March 26, 2025 16:01

github-actions bot removed the stale label Mar 26, 2025

hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from 7630693 to dbc00bb Compare March 27, 2025 02:57

hqbhoho requested review from chenjian2664 and ebyhr and removed request for chenjian2664 March 27, 2025 03:58

ebyhr reviewed Mar 31, 2025

View reviewed changes

hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from dbc00bb to ea695d8 Compare March 31, 2025 15:38

github-actions bot added the docs label Mar 31, 2025

hqbhoho requested a review from ebyhr April 1, 2025 00:56

ebyhr reviewed Apr 1, 2025

View reviewed changes

Add support column properties in base jdbc connector

26e3ed8

hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from ea695d8 to 3c514c5 Compare April 1, 2025 06:34

hqbhoho added 2 commits April 1, 2025 14:42

Add support create table with auto increment column

ae362ad

Unified format in BaseMySqlConnectorTest

abd364a

hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from 3c514c5 to abd364a Compare April 1, 2025 06:44

hqbhoho requested a review from ebyhr April 1, 2025 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support column properties with jdbc connector #25174

Support column properties with jdbc connector #25174

hqbhoho commented Feb 27, 2025 •

edited

Loading

github-actions bot commented Mar 20, 2025

hqbhoho commented Mar 27, 2025 •

edited

Loading

ebyhr left a comment

ebyhr Mar 31, 2025

ebyhr Mar 31, 2025

hqbhoho Apr 1, 2025

ebyhr Mar 31, 2025

ebyhr Mar 31, 2025

ebyhr Mar 31, 2025

ebyhr Apr 1, 2025

hqbhoho Apr 1, 2025

ebyhr Apr 1, 2025

ebyhr Apr 1, 2025

hqbhoho Apr 1, 2025

ebyhr Apr 1, 2025

hqbhoho Apr 1, 2025

ebyhr Apr 1, 2025

ebyhr Apr 1, 2025

ebyhr Apr 1, 2025

hqbhoho Apr 1, 2025

	"If column can auto increment",
	"Generate a unique identity for new rows",

Support column properties with jdbc connector #25174

Are you sure you want to change the base?

Support column properties with jdbc connector #25174

Conversation

hqbhoho commented Feb 27, 2025 • edited Loading

Description

Additional context and related issues

Release notes

github-actions bot commented Mar 20, 2025

hqbhoho commented Mar 27, 2025 • edited Loading

ebyhr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hqbhoho commented Feb 27, 2025 •

edited

Loading

hqbhoho commented Mar 27, 2025 •

edited

Loading