Skip to content

Support column properties with jdbc connector#25174

Closed
hqbhoho wants to merge 3 commits into
trinodb:masterfrom
hqbhoho:feature/support_column_properties_with_jdbc_connector
Closed

Support column properties with jdbc connector#25174
hqbhoho wants to merge 3 commits into
trinodb:masterfrom
hqbhoho:feature/support_column_properties_with_jdbc_connector

Conversation

@hqbhoho
Copy link
Copy Markdown
Contributor

@hqbhoho hqbhoho commented Feb 27, 2025

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.

@cla-bot cla-bot Bot added the cla-signed label Feb 27, 2025
@hqbhoho hqbhoho marked this pull request as draft February 27, 2025 01:36
@github-actions github-actions Bot added the mysql MySQL connector label Feb 27, 2025
@hqbhoho hqbhoho changed the title Feature/support column properties with jdbc connector Support column properties with jdbc connector Feb 27, 2025
@github-actions
Copy link
Copy Markdown

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

@github-actions github-actions Bot added the stale label Mar 20, 2025
@hqbhoho 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 hqbhoho marked this pull request as ready for review March 26, 2025 15:08
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from dbc00bb to 7630693 Compare March 26, 2025 16:01
@github-actions github-actions Bot removed the stale label Mar 26, 2025
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from 7630693 to dbc00bb Compare March 27, 2025 02:57
@hqbhoho
Copy link
Copy Markdown
Contributor Author

hqbhoho commented Mar 27, 2025

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?

@hqbhoho hqbhoho requested review from chenjian2664 and ebyhr and removed request for chenjian2664 March 27, 2025 03:58
Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please update MySQL connector's documentation.

Comment thread plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnector.java Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlColumnProperties.java Outdated
Comment thread plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlMetadata.java Outdated
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from dbc00bb to ea695d8 Compare March 31, 2025 15:38
@github-actions github-actions Bot added the docs label Mar 31, 2025
@hqbhoho hqbhoho requested a review from ebyhr April 1, 2025 00:56
Comment thread plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcColumnHandle.java Outdated
Comment thread plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcColumnHandle.java Outdated
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from ea695d8 to 3c514c5 Compare April 1, 2025 06:34
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from 3c514c5 to abd364a Compare April 1, 2025 06:44
@hqbhoho hqbhoho requested a review from ebyhr April 1, 2025 07:40
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it possible to add this into DefaultJdbcMetadata directly?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not add the auto increment info here, so we can use it in DefaultJdbcMetadata directly

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe different datasource will use different column properties, so different datasource will have their own getColumnMetadata from JdbcColumnHandle. If we are sure on how each datasource behave,we can unify them in JdbcColumnHandle#getColumnMetadata.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel like maybe we could make this filed to Optional<Boolean>, use empty to mark the column not support such property.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

autoIncrement only mark column can auto generate or not, it would be like nullable. It can use datasource Metadata to control support such property or not.

@hqbhoho hqbhoho requested a review from Copilot April 7, 2025 02:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Comment thread plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Outdated
@hqbhoho
Copy link
Copy Markdown
Contributor Author

hqbhoho commented Apr 7, 2025

@ebyhr PTAL and let me know if there's anything else to adjust.

Comment thread docs/src/main/sphinx/connector/mysql.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you move the section? Please separate a commit for moving.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Table properties or column properties are part of the table management. So I move it into Schema and table management section

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The requireNonNull isn't required in case of ImmutableSet.copyOf.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Unsupported column type for AUTO_INCREMENT: " + type

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getColumnMetadata -> toColumnMetadata

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This may overwrite the existing extra info. Please verify handle.getColumnMetadata().getExtraInfo() is empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have the test coverage about a table having primary_key in a different order?

CREATE TABLE test(a bigint NOT NULL, b bigint NOT NULL WITH (auto_increment = true), c bigint) WITH (primary_key = ARRAY['b', 'a']);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rename this variable to expected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from abd364a to 0539692 Compare April 10, 2025 12:23
@hqbhoho hqbhoho requested review from chenjian2664 and ebyhr April 10, 2025 13:18
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch 2 times, most recently from 6b56ada to dc120c9 Compare April 24, 2025 09:36
@hqbhoho
Copy link
Copy Markdown
Contributor Author

hqbhoho commented Apr 25, 2025

@ebyhr PTAL and let me know if there's anything else to adjust. Thanks

@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from dc120c9 to 8c8c3cc Compare May 7, 2025 10:51
@github-actions
Copy link
Copy Markdown

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

@github-actions github-actions Bot added the stale label May 28, 2025
@hqbhoho hqbhoho force-pushed the feature/support_column_properties_with_jdbc_connector branch from 8c8c3cc to 651c622 Compare May 29, 2025 02:58
@hqbhoho hqbhoho requested a review from Praveen2112 May 29, 2025 03:51
@hqbhoho
Copy link
Copy Markdown
Contributor Author

hqbhoho commented May 29, 2025

@ebyhr @chenjian2664 @Praveen2112 Could you PTAL at this PR ? Thanks

@github-actions github-actions Bot removed the stale label May 29, 2025
@github-actions
Copy link
Copy Markdown

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

@github-actions github-actions Bot added the stale label Jun 20, 2025
@github-actions
Copy link
Copy Markdown

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants