Copilot commented on code in PR #10191:
URL: https://github.com/apache/gravitino/pull/10191#discussion_r2883443364
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/updater/metrics/GravitinoMetricsUpdater.java:
##########
@@ -68,6 +68,14 @@ public void close() throws Exception {
}
}
+ void setMetricsRepositoryForTest(MetricsRepository metricsRepository) {
+ this.metricsStorage = metricsRepository;
+ }
+
+ MetricsRepository metricsRepositoryForTest() {
+ return metricsStorage;
+ }
Review Comment:
The newly added test-only hooks (`setMetricsRepositoryForTest` /
`metricsRepositoryForTest`) are unannotated, which makes it easy to
accidentally treat them as production API. In this module, similar test hooks
are typically marked with `@VisibleForTesting` (e.g., `Recommender` ctor
overload and `Updater#get*Updater`). Consider annotating these methods (and
optionally adding a short comment) to clarify intent.
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/integration/test/GravitinoTableMetaIT.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.maintenance.optimizer.integration.test;
+
+import
org.apache.gravitino.maintenance.optimizer.recommender.table.GravitinoTableMetadataProvider;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.Table;
+import org.apache.gravitino.rel.expressions.transforms.Transform;
+import org.apache.gravitino.rel.expressions.transforms.Transforms;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+public class GravitinoTableMetaIT extends GravitinoOptimizerEnvIT {
+ private GravitinoTableMetadataProvider tableMetadataProvider;
+
+ @BeforeAll
+ void init() {
+ this.tableMetadataProvider = new GravitinoTableMetadataProvider();
+ tableMetadataProvider.initialize(optimizerEnv);
+ }
+
+ @AfterAll
+ void closeResources() throws Exception {
+ if (tableMetadataProvider != null) {
+ tableMetadataProvider.close();
+ }
+ }
+
+ @Test
+ void testTableStatisticsUpdaterAndProvider() {
Review Comment:
The test name `testTableStatisticsUpdaterAndProvider` doesn't match what the
test is asserting (it only exercises `GravitinoTableMetadataProvider` and
verifies table/partitioning metadata). Renaming the test to reflect table
metadata behavior will make failures easier to interpret and keep the suite
searchable.
```suggestion
void testTableMetadataProviderTableAndPartitioningMetadata() {
```
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/integration/test/DummyTableStatisticsComputer.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.maintenance.optimizer.integration.test;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.List;
+import java.util.Map;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.maintenance.optimizer.api.common.MetricPoint;
+import org.apache.gravitino.maintenance.optimizer.api.common.PartitionEntry;
+import org.apache.gravitino.maintenance.optimizer.api.common.PartitionPath;
+import org.apache.gravitino.maintenance.optimizer.api.common.StatisticEntry;
+import
org.apache.gravitino.maintenance.optimizer.api.common.TableAndPartitionStatistics;
+import
org.apache.gravitino.maintenance.optimizer.api.updater.SupportsCalculateTableMetrics;
+import
org.apache.gravitino.maintenance.optimizer.api.updater.SupportsCalculateTableStatistics;
+import org.apache.gravitino.maintenance.optimizer.common.OptimizerEnv;
+import org.apache.gravitino.maintenance.optimizer.common.PartitionEntryImpl;
+import org.apache.gravitino.maintenance.optimizer.common.StatisticEntryImpl;
+import org.apache.gravitino.stats.StatisticValues;
+
+public class DummyTableStatisticsComputer
+ implements SupportsCalculateTableStatistics, SupportsCalculateTableMetrics
{
+
+ public static final String DUMMY_TABLE_STAT = "dummy-table-stat";
+ public static final String TABLE_STAT_NAME = "custom-dummy-table-stat-name";
+
+ @Override
+ public String name() {
+ return DUMMY_TABLE_STAT;
+ }
+
+ @Override
+ public void initialize(OptimizerEnv optimizerEnv) {}
+
+ @Override
+ public TableAndPartitionStatistics calculateTableStatistics(NameIdentifier
tableIdentifier) {
+ List<StatisticEntry<?>> tableStatistics =
+ List.of(new StatisticEntryImpl<>(TABLE_STAT_NAME,
StatisticValues.longValue(1L)));
+ Map<PartitionPath, List<StatisticEntry<?>>> partitionStatistics =
+ Map.of(
+ PartitionPath.of(getPartitionName()),
+ List.of(new StatisticEntryImpl<>(TABLE_STAT_NAME,
StatisticValues.longValue(2L))));
+ return new TableAndPartitionStatistics(tableStatistics,
partitionStatistics);
+ }
+
+ @Override
+ public List<MetricPoint> calculateTableMetrics(NameIdentifier
tableIdentifier) {
+ long timestampSeconds = System.currentTimeMillis() / 1000;
+ PartitionPath partitionPath = PartitionPath.of(getPartitionName());
+ return List.of(
+ MetricPoint.forTable(
+ tableIdentifier, TABLE_STAT_NAME, StatisticValues.longValue(1L),
timestampSeconds),
+ MetricPoint.forPartition(
+ tableIdentifier,
+ partitionPath,
+ TABLE_STAT_NAME,
+ StatisticValues.longValue(2L),
+ timestampSeconds));
+ }
+
+ @VisibleForTesting
+ public static List<PartitionEntry> getPartitionName() {
+ return List.of(new PartitionEntryImpl("p1", "v1"));
+ }
Review Comment:
`getPartitionName()` returns a `List<PartitionEntry>` (partition entries),
not a single partition name. This naming is misleading and makes call sites
harder to read (e.g., `PartitionPath.of(getPartitionName())`). Consider
renaming to something like `partitionEntries()` / `getPartitionEntries()` or
returning a `PartitionPath` directly.
##########
maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/updater/statistics/GravitinoStatisticsUpdater.java:
##########
@@ -153,4 +153,8 @@ public void close() throws Exception {
gravitinoClient.close();
}
}
+
+ void setGravitinoClientForTest(GravitinoClient gravitinoClient) {
+ this.gravitinoClient = gravitinoClient;
+ }
Review Comment:
`setGravitinoClientForTest` is a test-only hook added in main source but
isn't marked as such. Elsewhere in optimizer, test-only entry points are
commonly annotated with `@VisibleForTesting` to prevent accidental production
use. Consider adding `@VisibleForTesting` (and/or a brief comment) to make the
intent explicit.
--
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]
