Skip to content

Comments

Refactor the ContainerQueryRunner to utilize the JDBC driver for executing queries#23784

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
Joe-Abraham:cli
Oct 17, 2024
Merged

Refactor the ContainerQueryRunner to utilize the JDBC driver for executing queries#23784
tdcmeehan merged 1 commit intoprestodb:masterfrom
Joe-Abraham:cli

Conversation

@Joe-Abraham
Copy link
Contributor

@Joe-Abraham Joe-Abraham commented Oct 8, 2024

Description

Refactor the ContainerQueryRunner to utilize the JDBC driver for executing queries.

@Joe-Abraham Joe-Abraham force-pushed the cli branch 3 times, most recently from 9ec91a4 to 5338355 Compare October 10, 2024 08:58
Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Directionally this looks fine.

@Joe-Abraham Joe-Abraham force-pushed the cli branch 6 times, most recently from 3e41383 to 3765257 Compare October 11, 2024 11:37
@Joe-Abraham Joe-Abraham changed the title Refactor ContainerQueryRunner to use jdbi instead of cli Refactor the ContainerQueryRunner to utilize the JDBC driver for executing queries Oct 11, 2024
@Joe-Abraham Joe-Abraham marked this pull request as ready for review October 11, 2024 11:38
@Joe-Abraham Joe-Abraham requested a review from a team as a code owner October 11, 2024 11:38
@Joe-Abraham Joe-Abraham requested a review from tdcmeehan October 11, 2024 13:38
catch (InterruptedException e) {
throw new RuntimeException(e);
catch (SQLException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please throw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code.

Comment on lines 399 to 401
// Log or handle unknown type names
System.out.println("Unknown SQL type name: " + typeName);
return UnknownType.UNKNOWN;
Copy link
Contributor

Choose a reason for hiding this comment

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

Throw an error here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code.

@Joe-Abraham Joe-Abraham force-pushed the cli branch 2 times, most recently from 2193ae1 to 8f7e9e0 Compare October 16, 2024 13:14
@Joe-Abraham Joe-Abraham requested a review from tdcmeehan October 16, 2024 13:14
Connection connection = DriverManager.getConnection(url, "test", null);
statement = connection.createStatement();
}
catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is thrown here? If it's an IOException, can we let it simply throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an SQLException.

@tdcmeehan
Copy link
Contributor

Commit message is too long. Perhaps Use JDBC for ContainerQueryRunner.

@Joe-Abraham
Copy link
Contributor Author

Updated the commit message

@tdcmeehan tdcmeehan self-assigned this Oct 16, 2024
@tdcmeehan tdcmeehan merged commit 8c69bcb into prestodb:master Oct 17, 2024
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@Joe-Abraham Joe-Abraham deleted the cli branch November 28, 2024 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants