Copilot commented on code in PR #10094:
URL: https://github.com/apache/gravitino/pull/10094#discussion_r2867258327
##########
docs/trino-connector/configuration.md:
##########
@@ -13,7 +13,7 @@ license: "This software is licensed under the Apache License
version 2."
| trino.jdbc.user | string | admin
| The jdbc user name of current Trino.
| NO
| 0.5.1 |
| trino.jdbc.password | string | (none)
| The jdbc password of current Trino.
| NO
| 0.5.1 |
| gravitino.metadata.refresh-interval-seconds | integer | 10
| The `gravitino.metadata.refresh-interval-seconds` defines the interval in
seconds to refresh metadata from Gravitino server, the default value is 10
seconds.
| No | 0.9.0 |
-| gravitino.trino.skip-version-validation | boolean | false
| The `gravitino.trino.skip-version-validation` defines whether skip Trino
version validation or not. Note that Gravitino only supports Trino which
version between 435 and 439, other versions of Trino have not undergone
thorough testing, so there may be compatiablity problem if true.
| No | 1.0.0 |
+| gravitino.trino.skip-version-validation | boolean | false
| The `gravitino.trino.skip-version-validation` defines whether to skip Trino
version validation. Gravitino supports Trino versions between 435 and 462. If
this option is `true`, unsupported Trino versions can still be used, but
compatibility is not guaranteed.
| No | 1.0.0 |
Review Comment:
The supported Trino version range in this table (`435` to `462`) conflicts
with the updated connector modules/docs in this PR (now up to `472`). Please
update this description to match the actual supported ranges (e.g., `435-472`)
so users don’t get misled about version compatibility.
```suggestion
| gravitino.trino.skip-version-validation | boolean | false
| The `gravitino.trino.skip-version-validation` defines whether to skip
Trino version validation. Gravitino supports Trino versions between 435 and
472. If this option is `true`, unsupported Trino versions can still be used,
but compatibility is not guaranteed.
| No | 1.0.0 |
```
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/iceberg/IcebergMetadataAdapter.java:
##########
@@ -117,13 +118,12 @@ public GravitinoTable createTable(ConnectorTableMetadata
tableMetadata) {
toGravitinoTableProperties(
removeKeys(tableMetadata.getProperties(),
ICEBERG_PROPERTIES_TO_REMOVE));
- if (propertyMap.containsKey(ICEBERG_FORMAT_PROPERTY)) {
- String format = propertyMap.get(ICEBERG_FORMAT_PROPERTY).toString();
- properties.put(PROVIDER, format);
- if (propertyMap.containsKey((ICEBERG_FORMAT_VERSION_PROPERTY))) {
- properties.put(FORMAT_VERSION,
propertyMap.get(ICEBERG_FORMAT_VERSION_PROPERTY).toString());
- }
- }
+ properties.put(PROVIDER,
propertyMap.get(ICEBERG_FORMAT_PROPERTY).toString());
+ properties.put(
+ FORMAT_VERSION,
+ propertyMap
+ .getOrDefault(ICEBERG_FORMAT_VERSION_PROPERTY,
DEFAULT_ICEBERG_FORMAT_VERSION)
+ .toString());
Review Comment:
`propertyMap.get(ICEBERG_FORMAT_PROPERTY).toString()` will throw a
NullPointerException when the `format` table property is not explicitly
provided in `ConnectorTableMetadata` (defaults are typically applied by Trino
later, not necessarily present in `getProperties()`). Consider using
`getOrDefault(ICEBERG_FORMAT_PROPERTY, DEFAULT_ICEBERG_FORMAT)` (and similarly
for format version) or validating presence and throwing a clear
`TrinoException` if required.
##########
integration-test-common/docker-script/launch.sh:
##########
@@ -36,14 +36,20 @@ cd ${playground_dir}
LOG_DIR=../build/trino-ci-container-log
rm -fr $LOG_DIR
mkdir -p $LOG_DIR
-
-docker compose up -d
-
LOG_PATH=$LOG_DIR/trino-ci-docker-compose.log
-
echo "The docker compose log is: $LOG_PATH"
-nohup docker compose logs -f -t > $LOG_PATH &
+docker compose up -d
+
+# Stream logs directly to the log file (no console output).
+nohup docker compose logs -f -t | tee -a "$LOG_PATH" &
Review Comment:
The comment says “no console output”, but `tee -a "$LOG_PATH"` will still
write logs to stdout (and in CI can be very noisy). If the intent is to only
persist logs to the file, redirect `tee` stdout to `/dev/null` or replace the
pipeline with a direct redirect to the file.
```suggestion
nohup docker compose logs -f -t | tee -a "$LOG_PATH" >/dev/null &
```
--
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]
