Copilot commented on code in PR #9752:
URL: https://github.com/apache/gravitino/pull/9752#discussion_r2710723466
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##########
@@ -243,29 +250,46 @@ public static void main(String[] args) throws Exception {
TrinoQueryITBase.autoStart = autoStart;
TrinoQueryITBase.autoStartGravitino = autoStartGravitino;
- TrinoQueryIT testerRunner = new TrinoQueryIT();
- testerRunner.setup();
+ String trinoVersionList =
commandLine.getOptionValue("trino_version_list", "435");
+ String[] trinoVersions =
+ trinoVersionList.isEmpty() ? new String[] {""} :
trinoVersionList.split(",");
Review Comment:
The empty string handling logic is inconsistent. When
trinoVersionList.isEmpty() is true, it creates an array with a single empty
string, but then line 258 only sets the version if it's not empty. This means
if an empty string is passed, the default "435" version won't be used. Consider
either using the default value directly or ensuring the empty check is
consistent with the version assignment logic.
```suggestion
trinoVersionList.isEmpty() ? new String[] {"435"} :
trinoVersionList.split(",");
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/TrinoITContainers.java:
##########
@@ -58,13 +58,32 @@ public void launch(
boolean isTrinoConnectorTest,
int trinoWorkerNum)
throws Exception {
+ launch(gravitinoServerPort, hiveRuntimeVersion, isTrinoConnectorTest,
trinoWorkerNum, "435");
+ }
+
+ public void launch(
+ int gravitinoServerPort,
+ String hiveRuntimeVersion,
+ boolean isTrinoConnectorTest,
+ int trinoWorkerNum,
+ String trinoVersion)
+ throws Exception {
shutdown();
Map<String, String> env = new HashMap<>();
env.put("TRINO_WORKER_NUM", String.valueOf(trinoWorkerNum));
env.put("GRAVITINO_SERVER_PORT", String.valueOf(gravitinoServerPort));
env.put("HIVE_RUNTIME_VERSION", hiveRuntimeVersion);
env.put("TRINO_CONNECTOR_TEST", String.valueOf(isTrinoConnectorTest));
+ if (Strings.isNotEmpty(trinoVersion)) {
+ env.put("TRINO_VERSION", trinoVersion);
+ int version = Integer.parseInt(trinoVersion);
+ if (version >= 435 && version <= 439) {
+ env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
+ } else {
+ env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
Review Comment:
The version string is parsed to an integer without validation or error
handling. If a non-numeric version is provided (e.g., "latest", "nightly"),
parseInt will throw a NumberFormatException that is not caught, causing the
container launch to fail ungracefully. Consider adding validation with a
try-catch block and providing a clear error message if the version format is
invalid.
```suggestion
try {
int version = Integer.parseInt(trinoVersion);
if (version >= 435 && version <= 439) {
env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
} else {
env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
}
} catch (NumberFormatException e) {
throw new ContainerLaunchException(
"Invalid TRINO_VERSION value '" + trinoVersion + "'. It must be
a numeric version.", e);
```
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryIT.java:
##########
@@ -80,7 +81,8 @@ public class TrinoQueryIT extends TrinoQueryITBase {
@BeforeAll
public void setup() throws Exception {
- trinoQueryITBase = new TrinoQueryITBase(trinoWorkerNum);
+ allTestStatus.clear();
Review Comment:
Clearing the static allTestStatus map at the start of each setup() call can
cause issues when running multiple Trino versions in sequence. Since the map is
static and shared across all test runs, clearing it here will lose test results
from previous version runs. This should either be moved outside the loop in
TrinoQueryTestTool, or a separate map should be maintained for each version run.
```suggestion
```
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##########
@@ -92,6 +92,11 @@ public static void main(String[] args) throws Exception {
+ "Use a distributed cluster for integration testing when
necessary (value > 0), "
+ "otherwise fall back to a single-node setup with combined
coordinator-worker roles.");
+ options.addOption(
+ "trino_version_list",
+ true,
+ "Specify the Trino version list to test, example:
--trino_version_list=435,478");
Review Comment:
The help text description doesn't mention that the default value is "435"
when not specified. Consider adding this information to help users understand
the default behavior, for example: "Specify the Trino version list to test
(default: 435), example: --trino_version_list=435,478".
```suggestion
"Specify the Trino version list to test (default: 435), example:
--trino_version_list=435,478");
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/TrinoITContainers.java:
##########
@@ -58,13 +58,32 @@ public void launch(
boolean isTrinoConnectorTest,
int trinoWorkerNum)
throws Exception {
+ launch(gravitinoServerPort, hiveRuntimeVersion, isTrinoConnectorTest,
trinoWorkerNum, "435");
+ }
+
+ public void launch(
+ int gravitinoServerPort,
+ String hiveRuntimeVersion,
+ boolean isTrinoConnectorTest,
+ int trinoWorkerNum,
+ String trinoVersion)
+ throws Exception {
shutdown();
Map<String, String> env = new HashMap<>();
env.put("TRINO_WORKER_NUM", String.valueOf(trinoWorkerNum));
env.put("GRAVITINO_SERVER_PORT", String.valueOf(gravitinoServerPort));
env.put("HIVE_RUNTIME_VERSION", hiveRuntimeVersion);
env.put("TRINO_CONNECTOR_TEST", String.valueOf(isTrinoConnectorTest));
+ if (Strings.isNotEmpty(trinoVersion)) {
+ env.put("TRINO_VERSION", trinoVersion);
+ int version = Integer.parseInt(trinoVersion);
+ if (version >= 435 && version <= 439) {
+ env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
+ } else {
+ env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
+ }
Review Comment:
The conditional logic here is redundant - both branches set
TRINO_CONNECTOR_MODULE to the same value "trino-connector". Either this logic
is incomplete (perhaps different versions should use different modules), or the
entire if-else block can be removed. If different versions are meant to use
different connector modules, please implement the correct logic.
```suggestion
env.put("TRINO_CONNECTOR_MODULE", "trino-connector");
```
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##########
@@ -243,29 +250,46 @@ public static void main(String[] args) throws Exception {
TrinoQueryITBase.autoStart = autoStart;
TrinoQueryITBase.autoStartGravitino = autoStartGravitino;
- TrinoQueryIT testerRunner = new TrinoQueryIT();
- testerRunner.setup();
+ String trinoVersionList =
commandLine.getOptionValue("trino_version_list", "435");
+ String[] trinoVersions =
+ trinoVersionList.isEmpty() ? new String[] {""} :
trinoVersionList.split(",");
- if (commandLine.hasOption("gen_output")) {
- String catalogFileName = "catalog_" + catalog + "_prepare.sql";
- testerRunner.runOneTestSetAndGenOutput(testSetDir, catalogFileName,
testerId);
- System.out.println("The output file is generated successfully in the
path " + testSetDir);
- return;
- }
+ for (String trinoVersion : trinoVersions) {
+ if (!trinoVersion.isEmpty()) {
+ System.out.println("Testing Trino version: " + trinoVersion);
+ TrinoQueryIT.trinoVersion = trinoVersion.trim();
+ }
- if (testSet == null) {
- testerRunner.testSql();
- } else {
- testerRunner.testSql(testSetDir, catalog, testerId);
+ TrinoQueryIT testerRunner = new TrinoQueryIT();
+ try {
+ testerRunner.setup();
+
+ if (commandLine.hasOption("gen_output")) {
+ String catalogFileName = "catalog_" + catalog + "_prepare.sql";
+ testerRunner.runOneTestSetAndGenOutput(testSetDir,
catalogFileName, testerId);
+ System.out.println(
+ "The output file is generated successfully in the path " +
testSetDir);
Review Comment:
The gen_output option generates output files but doesn't break the loop. If
multiple Trino versions are specified, this will generate output for each
version, potentially overwriting the same output files. In the original code,
there was a return statement after gen_output (see removed lines). Consider
either breaking the loop after the first gen_output or documenting that
gen_output only makes sense with a single version.
```suggestion
"The output file is generated successfully in the path " +
testSetDir);
return;
```
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##########
@@ -243,29 +250,46 @@ public static void main(String[] args) throws Exception {
TrinoQueryITBase.autoStart = autoStart;
TrinoQueryITBase.autoStartGravitino = autoStartGravitino;
- TrinoQueryIT testerRunner = new TrinoQueryIT();
- testerRunner.setup();
+ String trinoVersionList =
commandLine.getOptionValue("trino_version_list", "435");
+ String[] trinoVersions =
+ trinoVersionList.isEmpty() ? new String[] {""} :
trinoVersionList.split(",");
- if (commandLine.hasOption("gen_output")) {
- String catalogFileName = "catalog_" + catalog + "_prepare.sql";
- testerRunner.runOneTestSetAndGenOutput(testSetDir, catalogFileName,
testerId);
- System.out.println("The output file is generated successfully in the
path " + testSetDir);
- return;
- }
+ for (String trinoVersion : trinoVersions) {
+ if (!trinoVersion.isEmpty()) {
+ System.out.println("Testing Trino version: " + trinoVersion);
+ TrinoQueryIT.trinoVersion = trinoVersion.trim();
+ }
- if (testSet == null) {
- testerRunner.testSql();
- } else {
- testerRunner.testSql(testSetDir, catalog, testerId);
+ TrinoQueryIT testerRunner = new TrinoQueryIT();
+ try {
+ testerRunner.setup();
+
+ if (commandLine.hasOption("gen_output")) {
+ String catalogFileName = "catalog_" + catalog + "_prepare.sql";
+ testerRunner.runOneTestSetAndGenOutput(testSetDir,
catalogFileName, testerId);
+ System.out.println(
+ "The output file is generated successfully in the path " +
testSetDir);
+ } else {
+ if (testSet == null) {
+ testerRunner.testSql();
+ } else {
+ testerRunner.testSql(testSetDir, catalog, testerId);
+ }
+ System.out.printf(
+ "Trino version %s testers have finished. Total:%s, Pass:
%s\n%s",
+ trinoVersion,
Review Comment:
When trinoVersion is empty (the default case), this will print "Trino
version testers have finished" with a blank version. This could be confusing.
Consider either using the default version value "435" in the output or handling
the empty case explicitly to print "Trino version 435 (default) testers have
finished".
```suggestion
String displayTrinoVersion =
trinoVersion.isEmpty() ? "435 (default)" : trinoVersion;
System.out.printf(
"Trino version %s testers have finished. Total:%s, Pass:
%s\n%s",
displayTrinoVersion,
```
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryTestTool.java:
##########
@@ -243,29 +250,46 @@ public static void main(String[] args) throws Exception {
TrinoQueryITBase.autoStart = autoStart;
TrinoQueryITBase.autoStartGravitino = autoStartGravitino;
- TrinoQueryIT testerRunner = new TrinoQueryIT();
- testerRunner.setup();
+ String trinoVersionList =
commandLine.getOptionValue("trino_version_list", "435");
+ String[] trinoVersions =
+ trinoVersionList.isEmpty() ? new String[] {""} :
trinoVersionList.split(",");
Review Comment:
There's no validation of the Trino version values in the list. Unlike
trinoWorkerNum which has validation (lines 232-246), the trinoVersion values
are not checked. Consider adding validation to ensure each version is a valid
integer and provide a helpful error message if invalid values are provided,
similar to the trinoWorkerNum validation pattern.
```suggestion
// Validate Trino version values to ensure they are valid integers
for (String rawTrinoVersion : trinoVersions) {
if (!rawTrinoVersion.isEmpty()) {
String trimmedVersion = rawTrinoVersion.trim();
try {
int parsedVersion = Integer.parseInt(trimmedVersion);
if (parsedVersion <= 0) {
System.out.println(
"Each value in trino_version_list must be a positive
integer, invalid value: "
+ rawTrinoVersion);
System.exit(1);
}
} catch (NumberFormatException e) {
System.out.println(
"Each value in trino_version_list must be an integer,
invalid value: "
+ rawTrinoVersion);
System.exit(1);
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
