Copilot commented on code in PR #9965:
URL: https://github.com/apache/gravitino/pull/9965#discussion_r2792547599
##########
trino-connector/integration-test/trino-test-tools/run_test_with_versions.sh:
##########
@@ -79,9 +79,9 @@ for entry in $trino_versions_map; do
# execute test
echo "Running test for Trino version: $trino_version with connector:
$trino_module_name"
echo "The args: $args"
-
+
sleep 5
-
$GRAVITINO_HOME_DIR/trino-connector/integration-test/trino-test-tools/trino_test.sh
$args
+
$GRAVITINO_HOME_DIR/trino-connector/integration-test/trino-test-tools/trino_integration_test.sh
$args
Review Comment:
This script now invokes `trino_integration_test.sh`, but the header comment
still says it wraps `trino_test.sh`. Please update the comment (and/or variable
names) so the documentation matches the actual script being executed.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/hive/HiveConnectorAdapter.java:
##########
@@ -56,6 +56,7 @@ public Map<String, String>
buildInternalConnectorConfig(GravitinoCatalog catalog
config.putAll(trinoProperty);
config.put("hive.metastore.uri", metastoreUri);
config.put("hive.security", "allow-all");
+ config.put("fs.hadoop.enabled", "true");
return config;
Review Comment:
`fs.hadoop.enabled` is being set after `config.putAll(trinoProperty)`, which
means it will override any user-provided `fs.hadoop.enabled` coming from
catalog properties despite the comment about put-order determining priority. If
this is meant to be a default, consider only setting it when absent (e.g.,
`putIfAbsent`) so users can override it.
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryIT.java:
##########
@@ -338,6 +338,7 @@ public void testSql() throws Exception {
String[] testSetNames =
Arrays.stream(TrinoQueryITBase.listDirectory(testsetsDir))
+ .filter(s -> new java.io.File(ITUtils.joinPath(testsetsDir,
s)).isDirectory())
.filter(s -> ciTestsets.isEmpty() || ciTestsets.contains(s))
Review Comment:
Avoid using fully-qualified class names inside method bodies here (`new
java.io.File(...)`). This codebase’s Java style prefers normal imports; please
import `java.io.File` (or reuse an existing import) and use `new File(...)`
instead.
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/TrinoITContainers.java:
##########
@@ -73,9 +71,11 @@ public void launch(
env.put("TRINO_VERSION", String.valueOf(trinoVersion));
}
if (trinoConnectorDir != null) {
- Path path = Paths.get(trinoConnectorDir);
- if (!Files.exists(path)) {
- throw new Exception("Provided GRAVITINO_TRINO_CONNECTOR_DIR '" + path
+ "' does not exist");
+ File dir = new File(trinoConnectorDir);
+ if (!dir.exists() || dir.list().length == 0) {
+ throw new Exception(
+ "Gravitino trino connector directory %s is not exist or empty"
+ .formatted(trinoConnectorDir));
}
Review Comment:
`dir.list()` can return null (e.g., when `trinoConnectorDir` exists but is
not a directory, or on I/O error). Calling `dir.list().length` can throw NPE
and will hide the real problem. Consider checking `dir.isDirectory()` first and
handling a null `list()` result (or using `listFiles()` and null-checking)
before checking emptiness.
```suggestion
if (!dir.exists() || !dir.isDirectory()) {
throw new Exception(
"Gravitino trino connector directory %s is not an existing
directory"
.formatted(trinoConnectorDir));
}
String[] connectorFiles = dir.list();
if (connectorFiles == null || connectorFiles.length == 0) {
throw new Exception(
"Gravitino trino connector directory %s is empty or not readable"
.formatted(trinoConnectorDir));
}
```
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergCatalogPropertyConverter.java:
##########
@@ -68,6 +68,7 @@ public Map<String, String>
gravitinoToEngineProperties(Map<String, String> prope
// The order of put operations determines the priority of parameters.
config.putAll(super.gravitinoToEngineProperties(properties));
config.putAll(stringStringMap);
+ config.put("fs.hadoop.enabled", "true");
return config;
Review Comment:
`fs.hadoop.enabled` is added after both
`super.gravitinoToEngineProperties()` and backend-specific properties, so it
will always override any explicit setting coming from the input `properties`
map. If the intent is to enable it by default for certain Trino
versions/backends, consider using a conditional (or `putIfAbsent`) so callers
can still override it.
--
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]
