-
Notifications
You must be signed in to change notification settings - Fork 1.2k
perf(spanner-jdbc): cache JDBC metadata query strings #14041
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
Changes from all commits
ad97128
62447b1
14bac61
f6db9ea
40d5d98
13f5a2a
37ac2c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,16 +20,20 @@ | |
| import com.google.auth.ServiceAccountSigner; | ||
| import com.google.auth.oauth2.UserCredentials; | ||
| import com.google.cloud.spanner.Dialect; | ||
| import com.google.cloud.spanner.ErrorCode; | ||
| import com.google.cloud.spanner.ResultSets; | ||
| import com.google.cloud.spanner.SpannerException; | ||
| import com.google.cloud.spanner.SpannerExceptionFactory; | ||
| import com.google.cloud.spanner.Struct; | ||
| import com.google.cloud.spanner.Type; | ||
| import com.google.cloud.spanner.Type.StructField; | ||
| import com.google.cloud.spanner.connection.Connection.InternalMetadataQuery; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
| import com.google.common.collect.ImmutableSet; | ||
| import java.io.BufferedReader; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.InputStreamReader; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.sql.Connection; | ||
| import java.sql.DatabaseMetaData; | ||
| import java.sql.ResultSet; | ||
|
|
@@ -40,6 +44,8 @@ | |
| import java.util.Collections; | ||
| import java.util.Properties; | ||
| import java.util.Scanner; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentMap; | ||
|
|
||
| /** {@link DatabaseMetaData} implementation for Cloud Spanner */ | ||
| class JdbcDatabaseMetaData extends AbstractJdbcWrapper implements DatabaseMetaData { | ||
|
|
@@ -50,24 +56,36 @@ class JdbcDatabaseMetaData extends AbstractJdbcWrapper implements DatabaseMetaDa | |
| private static final String PRODUCT_NAME = "Google Cloud Spanner"; | ||
| private static final String POSTGRESQL_PRODUCT_NAME = PRODUCT_NAME + " PostgreSQL"; | ||
|
|
||
| private static final ConcurrentMap<String, String> SQL_CACHE = new ConcurrentHashMap<>(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cache is unbounded but negligible risk since metadata are bounded. |
||
|
|
||
| @VisibleForTesting | ||
| static String readSqlFromFile(String filename, Dialect dialect) { | ||
| InputStream in; | ||
| switch (dialect) { | ||
| case POSTGRESQL: | ||
| in = JdbcDatabaseMetaData.class.getResourceAsStream("postgresql/" + filename); | ||
| break; | ||
| case GOOGLE_STANDARD_SQL: | ||
| default: | ||
| in = JdbcDatabaseMetaData.class.getResourceAsStream(filename); | ||
| static String readSqlFromFile(String filename, Dialect dialect) throws SQLException { | ||
| try { | ||
| return SQL_CACHE.computeIfAbsent( | ||
| filename + "/" + dialect, (key) -> loadSqlFromFile(filename, dialect)); | ||
| } catch (SpannerException e) { | ||
| throw JdbcSqlExceptionFactory.of(e); | ||
| } | ||
| } | ||
|
olavloite marked this conversation as resolved.
olavloite marked this conversation as resolved.
|
||
|
|
||
| private static String loadSqlFromFile(String filename, Dialect dialect) { | ||
| String resourcePath = dialect == Dialect.POSTGRESQL ? "postgresql/" + filename : filename; | ||
| InputStream in = JdbcDatabaseMetaData.class.getResourceAsStream(resourcePath); | ||
| if (in == null) { | ||
| throw SpannerExceptionFactory.newSpannerException( | ||
| ErrorCode.NOT_FOUND, "Resource not found: " + resourcePath); | ||
| } | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(in)); | ||
| StringBuilder builder = new StringBuilder(); | ||
| try (Scanner scanner = new Scanner(reader)) { | ||
| try (InputStream input = in; | ||
| InputStreamReader reader = new InputStreamReader(input, StandardCharsets.UTF_8); | ||
| Scanner scanner = new Scanner(reader)) { | ||
| while (scanner.hasNextLine()) { | ||
| String line = scanner.nextLine(); | ||
| builder.append(line).append("\n"); | ||
| } | ||
| } catch (IOException e) { | ||
| throw SpannerExceptionFactory.newSpannerException( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: earlier there was a chance of NPE, and now better Just wanted to fyi since the PR title did not mention it |
||
| ErrorCode.INTERNAL, "Could not read SQL file " + resourcePath, e); | ||
| } | ||
| return builder.toString(); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.