Skip to content
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.

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

Attempt to Refactor identify mapping to use in non jdbc connector #17996

Conversation

vlad-lyutenko
Copy link
Contributor

@vlad-lyutenko vlad-lyutenko commented Jun 21, 2023

Description

To raise discussion

Additional context and related issues

Release notes

( ) This is not user-visible or 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:

# Section
* Fix some things. ({issue}`issuenumber`)

@kokosing
Copy link
Member

Please rebase

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch 2 times, most recently from 9085d15 to 372de04 Compare June 24, 2023 10:49
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch 14 times, most recently from 2119498 to 1565882 Compare June 30, 2023 10:58
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

I would squash these commits.

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch 4 times, most recently from b3c55ce to 316906a Compare July 2, 2023 17:51
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

LGTM % comments and review from others.

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch 2 times, most recently from d9fdf70 to cba3029 Compare July 3, 2023 13:54
@vlad-lyutenko vlad-lyutenko requested a review from Praveen2112 July 3, 2023 14:10
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch 2 times, most recently from eb1dddf to 68bd938 Compare July 4, 2023 09:34
@vlad-lyutenko vlad-lyutenko requested a review from krvikash July 4, 2023 12:46
Copy link
Contributor

@krvikash krvikash left a comment

Choose a reason for hiding this comment

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

LGTM

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch from 68bd938 to 61e0379 Compare July 5, 2023 10:26
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% nits

@@ -1440,4 +1448,9 @@ private static ColumnMetadata getPageSinkIdColumn(List<String> otherColumnNames)
}
return new ColumnMetadata(columnName, TRINO_PAGE_SINK_ID_COLUMN_TYPE);
}

public RemoteIdentifiers getRemoteIdentifiers(Connection connection)
Copy link
Member

Choose a reason for hiding this comment

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

protected or private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cannot, PhoenixMetadata need it:

    private String toRemoteSchemaName(ConnectorSession session, String schemaName)
    {
        try (Connection connection = phoenixClient.getConnection(session)) {
            return identifierMapping.toRemoteSchemaName(phoenixClient.getRemoteIdentifiers(connection), session.getIdentity(), schemaName);
        }
        catch (SQLException e) {
            throw new TrinoException(PHOENIX_METADATA_ERROR, "Couldn't get casing for the schema name", e);
        }
    }

public static class JdbcRemoteIdentifiersFactory
{
private final BaseJdbcClient baseJdbcClient;
// cache this field to omit metadata check/call for every remote identifiers call
Copy link
Member

Choose a reason for hiding this comment

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

it it is about LazyConnection, to not populate (create) connection each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, connection is already created when we pass it to:

public JdbcRemoteIdentifiers createJdbcRemoteIdentifies(Connection connection)
        {
            return new JdbcRemoteIdentifiers(baseJdbcClient, connection, storesUpperCaseIdentifiers(connection));
        }

we just don't want to call this :

DatabaseMetaData metadata = connection.getMetaData();
                storesUpperCaseIdentifiers = metadata.storesUpperCaseIdentifiers();

all the time when we call createJdbcRemoteIdentifies

@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch 2 times, most recently from bc74cdf to 6b85799 Compare July 6, 2023 15:06
And move identify mapping to plugin toolkit
@vlad-lyutenko vlad-lyutenko force-pushed the vlad-lyutenko/non-jdbc-identifier-mapping branch from 6b85799 to 09cbeb5 Compare July 7, 2023 09:33
@kokosing kokosing merged commit 3402a6b into trinodb:master Jul 8, 2023
@github-actions github-actions bot added this to the 422 milestone Jul 8, 2023
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.

3 participants