[#3630] feat(trino-connector): Support insert and select operation on Gravitino catalog #3631
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mchades
reviewed
May 29, 2024
| "https://repo1.maven.org/maven2/mysql/mysql-connector-java/8.0.26/mysql-connector-java-8.0.26.jar"; | ||
|
|
||
| private static final String DOWNLOAD_POSTGRESQL_JDBC_DRIVER_URL = | ||
| "https://jdbc.postgresql.org/download/postgresql-42.7.0.jar"; |
There was a problem hiding this comment.
why don't we need this before?
There was a problem hiding this comment.
I'm not sure at the moment. I disable the tester of TrinoConnectorIT.
There was a problem hiding this comment.
what's the bug directory means?
There was a problem hiding this comment.
The EXPLAIN command cannot show the scanner details about query optimization. I will debug the problem.
integration-test/trino-it/init/trino/config/catalog/gravitino.properties
Outdated
Show resolved
Hide resolved
...-connector/src/main/java/com/datastrato/gravitino/trino/connector/GravitinoColumnHandle.java
Outdated
Show resolved
Hide resolved
mchades
reviewed
May 30, 2024
...ion-test-common/src/test/java/com/datastrato/gravitino/integration/test/util/AbstractIT.java
Outdated
Show resolved
Hide resolved
mchades
reviewed
May 30, 2024
| JdbcDriverDownloader.downloadJdbcDriver(DOWNLOAD_JDBC_DRIVER_URL, tmpPath.toString()); | ||
| Path tmpPath = Paths.get(gravitinoHome, relativeDeployLibsPath); | ||
| JdbcDriverDownloader.downloadJdbcDriver(DOWNLOAD_MYSQL_JDBC_DRIVER_URL, tmpPath.toString()); | ||
| JdbcDriverDownloader.downloadJdbcDriver( |
There was a problem hiding this comment.
You should remove other download actions in the sub-classes
mchades
reviewed
May 30, 2024
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @Disabled |
There was a problem hiding this comment.
why disable this test? please add a todo comment or open a new issue to track this
There was a problem hiding this comment.
It's not work now. i will fix later
yuqi1129
reviewed
May 30, 2024
| @@ -92,24 +93,36 @@ public static GravitinoConnectorPluginManager instance(ClassLoader classLoader) | |||
| } | |||
| synchronized (GravitinoConnectorPluginManager.class) { | |||
| if (instance == null) { | |||
| if (!APP_CLASS_LOADER_NAME.equals(classLoader.getName())) { | |||
There was a problem hiding this comment.
Is the name AppClassLoader app?
| @@ -0,0 +1,48 @@ | |||
| /* | |||
| * Copyright 2023 Datastrato Pvt Ltd. | |||
There was a problem hiding this comment.
Please check the version wholely
| } | ||
| } | ||
|
|
||
| static TypeManager createTypeManger(ClassLoader classLoader) { |
|
|
||
| objectMapper.registerModule(module); | ||
| return objectMapper; | ||
| } catch (Exception e) {; |
| objectMapper.registerModule(new RecordAutoDetectModule()); | ||
|
|
||
| // Handle serialization for plugin classes | ||
| registerHandleSerializationModule(objectMapper, classLoader); |
| @@ -141,40 +155,40 @@ private void loadPlugin(String pluginPath, String pluginName) { | |||
| "org.openjdk.jol.", | |||
| "io.opentelemetry.api.", | |||
| "io.opentelemetry.context.")); | |||
| pluginClassLoaders.put(pluginName, (ClassLoader) pluginClassLoader); | |||
|
|
|||
| ServiceLoader<Plugin> serviceLoader = | |||
There was a problem hiding this comment.
Why move this logic here? It's in the createConnector before.
There was a problem hiding this comment.
Previously, the class only supported loading plugins from a directory, but in the test environment, we don't have a plugin directory. However, we can create a plugin immediately. Therefore, we have added a new interface, installPlugin(name, plugin), and then refactored the code to move the plugin loading from the createConnector function.
yuqi1129
approved these changes
May 30, 2024
diqiu50
added a commit
to diqiu50/gravitino
that referenced
this pull request
May 30, 2024
…ion on Gravitino catalog (apache#3631) ### What changes were proposed in this pull request? 1. Support insert and select operation on Gravitino catalog 2. Support query optimize 3. Upgrade CI docker environment ### Why are the changes needed? Fix: apache#3630 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? UT IT
diqiu50
added a commit
to diqiu50/gravitino
that referenced
this pull request
May 30, 2024
…ion on Gravitino catalog (apache#3631) 1. Support insert and select operation on Gravitino catalog 2. Support query optimize 3. Upgrade CI docker environment Fix: apache#3630 NO UT IT
diqiu50
added a commit
to diqiu50/gravitino
that referenced
this pull request
May 30, 2024
…ion on Gravitino catalog (apache#3631) 1. Support insert and select operation on Gravitino catalog 2. Support query optimize 3. Upgrade CI docker environment Fix: apache#3630 NO UT IT
diqiu50
added a commit
to diqiu50/gravitino
that referenced
this pull request
May 31, 2024
…ion on Gravitino catalog (apache#3631) 1. Support insert and select operation on Gravitino catalog 2. Support query optimize 3. Upgrade CI docker environment Fix: apache#3630 NO UT IT
jerryshao
pushed a commit
that referenced
this pull request
May 31, 2024
jerryshao
pushed a commit
that referenced
this pull request
May 31, 2024
… Gravitino catalog (#3631) ### What changes were proposed in this pull request? 1. Support insert and select operation on Gravitino catalog 2. Support query optimize 3. Upgrade CI docker environment ### Why are the changes needed? Fix: #3630 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? UT IT
diqiu50
added a commit
to diqiu50/gravitino
that referenced
this pull request
Jun 13, 2024
…ion on Gravitino catalog (apache#3631) ### What changes were proposed in this pull request? 1. Support insert and select operation on Gravitino catalog 2. Support query optimize 3. Upgrade CI docker environment ### Why are the changes needed? Fix: apache#3630 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? UT IT
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #3630
Does this PR introduce any user-facing change?
NO
How was this patch tested?
UT IT