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.

Sign up for GitHub

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

Jump to bottom

[#233] test (trino-connector): Add SQL interface tests for trino-connector. #524

Merged
merged 4 commits into from
Oct 19, 2023
Merged

[#233] test (trino-connector): Add SQL interface tests for trino-connector. #524

merged 4 commits into from
Oct 19, 2023

Conversation

diqiu50
Copy link
Contributor

@diqiu50 diqiu50 commented Oct 17, 2023 •
edited by jerryshao
Loading

What changes were proposed in this pull request?

Add SQL interface end to end UT for trino-connector. It uses the Graviton connector to load the memory catalog and execute basic SQL tests.

Why are the changes needed?

Fix: #523 Add SQL interface end to end UT for trino-connector

@diqiu50 diqiu50 self-assigned this Oct 17, 2023
@github-actions
Copy link

github-actions bot commented Oct 17, 2023 •
edited
Loading

Code Coverage Report

Overall Project 66.8% -0.11% 🟢
Files changed 83.21% 🟢

Module Coverage
trino-connector 82.9% -1.54% 🟢
Files
Module File Coverage
trino-connector MemoryMetadataAdapter.java 100% 🟢
MemoryConnectorAdapter.java 100% 🟢
GravitinoPlugin.java 100% 🟢
CatalogConnectorAdapter.java 100% 🟢
GravitinoMetadata.java 98.34% 🟢
GravitinoColumn.java 96.08% 🟢
DataTypeTransformer.java 95.26% 🟢
GravitinoConnectorFactory.java 90.7% 🟢
CatalogConnectorMetadataAdapter.java 83.5% -16.5% 🟢
CatalogConnectorFactory.java 63.89% 🟢
HiveConnectorAdapter.java 19.4% -11.94% 🔴
HiveMetadataAdapter.java 0% -83.33% 🔴

yuqi1129
yuqi1129 reviewed Oct 17, 2023
View reviewed changes
...in/java/com/datastrato/graviton/trino/connector/catalog/CatalogConnectorMetadataAdapter.java Outdated Show resolved Hide resolved
...main/java/com/datastrato/graviton/trino/connector/catalog/memory/MemoryConnectorAdapter.java Outdated Show resolved Hide resolved
.../main/java/com/datastrato/graviton/trino/connector/catalog/memory/MemoryMetadataAdapter.java Outdated Show resolved Hide resolved
...o-connector/src/test/java/com/datastrato/graviton/trino/connector/TestGravitonConnector.java Outdated Show resolved Hide resolved
...o-connector/src/test/java/com/datastrato/graviton/trino/connector/TestGravitonConnector.java Outdated Show resolved Hide resolved
...o-connector/src/test/java/com/datastrato/graviton/trino/connector/TestGravitonConnector.java Outdated Show resolved Hide resolved
@jerryshao
Copy link
Contributor

Is it ready for review?

@diqiu50
Copy link
Contributor Author

diqiu50 commented Oct 18, 2023

Is it ready for review?

Yes

@jerryshao
Copy link
Contributor

@yuqi1129 can you please help to review?

jerryshao
jerryshao reviewed Oct 19, 2023
View reviewed changes
build.gradle.kts
@@ -51,7 +55,11 @@ subprojects {
tasks.configureEach<Test> {
val skipTests = project.hasProperty("skipTests")
if (!skipTests) {
useJUnitPlatform()
if (project.name == "trino-connector") {
useTestNG()
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason to use testNG?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trino connector testing framework requires the use of TestNG.

...-connector/src/test/java/com/datastrato/gravitino/trino/connector/util/GravitinoRestApi.java
import java.util.Map;

@Path("/")
public class GravitinoRestApi {
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 the usage of this class?

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 simulates a Gravitino server to support Trino connector operations. To enable the Trino Connector to perform full functionality tests using the embedded memory connector within Trino.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worrying that if we change the protocol in Gravitino server, it should also be changed here, otherwise it will not align and lead to failure, which makes this code hard to maintain, do we have any better solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A better solution is to simulate some interfaces of GravitinoClient, GravitinoMetalake, and RelationalCatalog for the Trino connector. If there were a Graviton JDBC interface, it would make the implementation much easier. We will improve it later.

yuqi1129
yuqi1129 reviewed Oct 19, 2023
View reviewed changes
...connector/src/test/java/com/datastrato/gravitino/trino/connector/TestGravitinoConnector.java

public class TestGravitinoConnector extends BaseConnectorTest {
@Parameters({"-Xmx1G"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to limit the memory usage deliberately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Trino connector test need about 1G memory to run

...n/java/com/datastrato/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java Outdated
/** Normalize gravitino attributes for trino */
protected Map<String, Object> normalizeProperties(
Map<String, String> properties, List<PropertyMetadata<?>> propertyTemplate) {
// TODO yuhui redo this function once graviton table properties are supported..
Copy link
Contributor

Choose a reason for hiding this comment

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

graviton -> gravitino

jerryshao
jerryshao reviewed Oct 19, 2023
View reviewed changes
trino-connector/build.gradle.kts Outdated
testImplementation(libs.trino.testing) {
exclude("org.apache.logging.log4j")
}
testImplementation("io.trino:trino-memory:426") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you define this dependency in version file?

...n/java/com/datastrato/gravitino/trino/connector/catalog/CatalogConnectorMetadataAdapter.java Outdated
DataTypeTransformer.getGravitinoType(column.getType(), column.isNullable()),
index,
column.getComment()));
index++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the difference between i and index, why don't you directly use i?

...-connector/src/test/java/com/datastrato/gravitino/trino/connector/util/GravitinoRestApi.java
import java.util.Map;

@Path("/")
public class GravitinoRestApi {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worrying that if we change the protocol in Gravitino server, it should also be changed here, otherwise it will not align and lead to failure, which makes this code hard to maintain, do we have any better solution?

@jerryshao
Copy link
Contributor

I have no further comments, @yuqi1129 please also take a look again.

@jerryshao
Copy link
Contributor

BTW, the code need to rebase.

yuqi1129
yuqi1129 reviewed Oct 19, 2023
View reviewed changes
gradle/libs.versions.toml Outdated
@@ -30,6 +30,7 @@ trino = '426'
spark = "3.4.1"
scala-collection-compat = "2.7.0"
sqlite-jdbc = "3.42.0.0"
testng = "7.4.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does trino-connector use this version? I noticed that there is a vulnerability for this version.

Copy link
Contributor Author

@diqiu50 diqiu50 Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I change it to 7.7.1

@yuqi1129
Copy link
Contributor

The rest are LGTM.

yuqi1129
yuqi1129 approved these changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM

@jerryshao jerryshao merged commit 5f456bf into apache:main Oct 19, 2023
2 checks passed
@diqiu50 diqiu50 deleted the graviton-connector-ut branch October 25, 2023 09:48
jerryshao pushed a commit that referenced this pull request Oct 26, 2023
@diqiu50
[#523] test (trino-connector): Add SQL interface tests for trino-conn…
c956e43 
…ector. (#524)

### What changes were proposed in this pull request?

Add SQL interface end to end UT for trino-connector. It uses the
Graviton connector to load the memory catalog and execute basic SQL
tests.

### Why are the changes needed?

Fix: #523 Add SQL interface end to end UT for trino-connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerryshao jerryshao jerryshao left review comments

@yuqi1129 yuqi1129 yuqi1129 approved these changes

Assignees

@diqiu50 diqiu50

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Add SQL interface end to end UT for trino-connector
3 participants
@diqiu50 @jerryshao @yuqi1129