[#1573] feat(spark-connector): Spark regression test system #3933

FANNG1 · 2024-06-21T03:33:58Z

What changes were proposed in this pull request?

add Spark regression test system to do Regression test for SparkSQL.

hive, contains SQLs in https://datastrato.ai/docs/0.5.1/spark-connector/spark-catalog-hive
iceberg, contains SQLs in https://datastrato.ai/docs/0.5.1/spark-connector/spark-catalog-iceberg
tpcds, contains all queries, and the data is about scala 0.01 to reduce the size.

Why are the changes needed?

Fix: #1573

Does this PR introduce any user-facing change?

no

How was this patch tested?

add IT

FANNG1 · 2024-06-27T01:09:32Z

@yuqi1129 @diqiu50 @jerryshao @qqqttt123 , please help to review when you free, thanks

diqiu50 · 2024-06-27T02:08:19Z

What changes were proposed in this pull request?

add Spark regression test system to do Regression test for SparkSQL.

hive, contains SQLs in https://datastrato.ai/docs/0.5.1/spark-connector/spark-catalog-hive

iceberg, contains SQLs in https://datastrato.ai/docs/0.5.1/spark-connector/spark-catalog-iceberg

tpcds, contains all queries, and the data is about scala 0.01 to reduce the size.

Why are the changes needed?

Fix: #1573

Does this PR introduce any user-facing change?

no

How was this patch tested?

add IT

The links are not accessed

FANNG1 · 2024-06-27T02:17:07Z

What changes were proposed in this pull request?

add Spark regression test system to do Regression test for SparkSQL.

hive, contains SQLs in https://datastrato.ai/docs/0.5.1/spark-connector/spark-catalog-hive

iceberg, contains SQLs in https://datastrato.ai/docs/0.5.1/spark-connector/spark-catalog-iceberg

tpcds, contains all queries, and the data is about scala 0.01 to reduce the size.

Why are the changes needed?

Fix: #1573

Does this PR introduce any user-facing change?

no

How was this patch tested?

add IT

The links are not accessed

I could access the links, which links are not accessed? could you try again?

...est/java/com/datastrato/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java

FANNG1 · 2024-07-31T08:37:17Z

@diqiu50 @yuqi1129 @jerryshao , do you have time to review this?

jerryshao · 2024-07-31T09:37:41Z

I'm not sure if you add docs about how to test it or not, if not please add the docs.

...mon/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/CatalogType.java

...mon/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/QueryOutput.java

...rc/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java

...src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkTestConfig.java

jerryshao · 2024-07-31T10:16:47Z

...src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkTestConfig.java

+  public String getWarehouseLocation() {
+    return get(WAREHOUSE_DIR);
+  }
+}


Do we really need configurations for tests? I feel think we don't have to formalize them just for test, WDYT?

I prefer to keep the configs to control the test behavior better.

If you want to keep this configuration, you should have a better name to clarify its scope and usage, currently I feel the name is randomly picked, some of them are not so meaningful.

...common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/TestCase.java

jerryshao · 2024-07-31T10:20:18Z

spark-connector/v3.3/spark/build.gradle.kts

@@ -144,10 +144,14 @@ tasks.test {

  val skipITs = project.hasProperty("skipITs")
  val skipSparkITs = project.hasProperty("skipSparkITs")
+  val skipSparkSQLITs = project.hasProperty("skipSparkSQLITs")


Do we need a configuration to enable or disable this test?

I feel like there are too many configurations to control whether to run something or not, do we have better solutions?

I would incline to use gradle to control whether to run this or not, by default we would not run these SQL tests.

I tried to define a specific task to run the test, but failed to initIntegrationTest. So I'd like to define a enableSparkSQLTests flag to enable the test explicitly, WDYT?

tasks.register("testSQL") { dependsOn("compileTestJava") doLast { javaexec { mainClass.set("org.apache.gravitino.spark.connector.integration.test.sql.SparkSQLRegressionTest33") classpath = sourceSets["test"].runtimeClasspath environment("GRAVITINO_CI_HIVE_DOCKER_IMAGE", "datastrato/gravitino-ci-hive:0.1.13") // failed to init initIntegrationTest in javaexec val init = project.extra.get("initIntegrationTest") as (Test) -> Unit init(this) } } }

...mon/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/CatalogType.java

yuqi1129 · 2024-07-31T09:56:31Z

...mon/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/CatalogType.java

+    switch (str.toLowerCase()) {
+      case "hive":
+        return HIVE;
+      case "lakehouse-iceberg":
+        return ICEBERG;
+      default:
+        return UNKNOWN;
+    }


Suggested change

switch (str.toLowerCase()) {

case "hive":

return HIVE;

case "lakehouse-iceberg":

return ICEBERG;

default:

return UNKNOWN;

}

for (CatalogType type : CatalogType.values())

if (type.name.equals(str.toUpperCase())) {

//....

}

}

LAKEHOUSE-ICEBERG, couldn't be used as enum values

...rc/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java

yuqi1129 · 2024-08-01T01:39:08Z

...rc/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java

+  private static File stringToFile(Path path, String str) throws IOException {
+    File file = path.toFile();
+    try (PrintWriter out = new PrintWriter(file, StandardCharsets.UTF_8.toString())) {
+      out.write(str);
+    }
+    return file;
+  }
+
+  private String fileToString(Path filePath) throws IOException {
+    return new String(Files.readAllBytes(filePath), StandardCharsets.UTF_8);
+  }


FileUtils.writeStringToFile(); FileUtils.readFileToString()

yuqi1129 · 2024-08-01T01:41:44Z

...t/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkSQLRegressionTest.java

+ * "com.datastrato.gravitino.spark.connector.integration.test.sql.SparkSQLRegressionTest"
+ * -PconfigFile=/xxx/xx
+ */
+@Tag("gravitino-docker-it")


gravitino-docker-test

yuqi1129 · 2024-08-01T01:44:09Z

...src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SparkTestConfig.java

+          .toString();
+
+  private static final ConfigEntry<String> TEST_BASE_DIR =
+      new ConfigBuilder("gravitino.test.sql.dir")


gravitino.test.sql.dir -> gravitino.spark.test.dir

FANNG1 · 2024-08-01T12:51:44Z

I'm not sure if you add docs about how to test it or not, if not please add the docs.

update document

jerryshao · 2024-08-02T07:01:54Z

@yuqi1129 would you please take another look?

yuqi1129 · 2024-08-02T07:04:06Z

...mon/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/CatalogType.java

+    }
+  }
+
+  // To find the catalog type from the directory name, the first name in directory match CatalogType


the first non-unknown CatalogType from parent to child determines the catalog type.

yuqi1129 · 2024-08-02T07:11:24Z

Others LGTM

FANNG1 · 2024-08-02T08:28:54Z

add support exclude test resources from test jar

FANNG1 marked this pull request as draft June 21, 2024 03:34

FANNG1 force-pushed the r-test branch 11 times, most recently from a6923a2 to 744990b Compare June 26, 2024 15:25

FANNG1 changed the title ~~[SIP] Spark Regression test~~ [#1573] feat(spark-connector): Spark regression test system Jun 27, 2024

FANNG1 marked this pull request as ready for review June 27, 2024 01:08

diqiu50 reviewed Jun 27, 2024

View reviewed changes

...est/java/com/datastrato/gravitino/spark/connector/integration/test/sql/SparkQueryRunner.java Outdated Show resolved Hide resolved

FANNG1 force-pushed the r-test branch 2 times, most recently from 8d6d2ec to 2cb407d Compare July 7, 2024 11:46

FANNG1 force-pushed the r-test branch from a898625 to 99f750e Compare July 24, 2024 04:02

FANNG1 added 2 commits July 24, 2024 15:02

spark test framework

9c6ca6e

polish

5b8b124

FANNG1 force-pushed the r-test branch from 99f750e to 5b8b124 Compare July 24, 2024 07:07

jerryshao reviewed Jul 31, 2024

View reviewed changes

polish

9fc0f26

yuqi1129 reviewed Aug 1, 2024

View reviewed changes

polish

761d5e0

FANNG1 added 5 commits August 1, 2024 20:00

add doc

9d494ae

modify github ci

bb156eb

polish

f767f99

polish

fac4d08

polish

b4b4e81

yuqi1129 reviewed Aug 2, 2024

View reviewed changes

exclude test resources from jar

05ba96b

polish

a0dd07d

jerryshao approved these changes Aug 2, 2024

View reviewed changes

jerryshao assigned FANNG1 Aug 2, 2024

jerryshao merged commit f288d91 into apache:main Aug 2, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1573] feat(spark-connector): Spark regression test system #3933

[#1573] feat(spark-connector): Spark regression test system #3933

FANNG1 commented Jun 21, 2024 •

edited

Loading

FANNG1 commented Jun 27, 2024

diqiu50 commented Jun 27, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Jun 27, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Jul 31, 2024

jerryshao commented Jul 31, 2024

jerryshao Jul 31, 2024

FANNG1 Aug 1, 2024

jerryshao Aug 1, 2024

FANNG1 Aug 1, 2024

jerryshao Jul 31, 2024

jerryshao Jul 31, 2024

FANNG1 Aug 1, 2024

yuqi1129 Jul 31, 2024

FANNG1 Aug 1, 2024

yuqi1129 Aug 1, 2024

FANNG1 Aug 1, 2024

yuqi1129 Aug 1, 2024

FANNG1 Aug 1, 2024

yuqi1129 Aug 1, 2024

FANNG1 Aug 1, 2024

FANNG1 commented Aug 1, 2024

jerryshao commented Aug 2, 2024

yuqi1129 Aug 2, 2024

FANNG1 Aug 2, 2024

yuqi1129 commented Aug 2, 2024

FANNG1 commented Aug 2, 2024

[#1573] feat(spark-connector): Spark regression test system #3933

[#1573] feat(spark-connector): Spark regression test system #3933

Conversation

FANNG1 commented Jun 21, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Jun 27, 2024

diqiu50 commented Jun 27, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Jun 27, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

FANNG1 commented Jul 31, 2024

jerryshao commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Aug 1, 2024

jerryshao commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 commented Aug 2, 2024

FANNG1 commented Aug 2, 2024

FANNG1 commented Jun 21, 2024 •

edited

Loading