This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new aa92227f8d [#14674] fix(maintenance): Ignore table-level stats for
partitioned strategies (#14675)
aa92227f8d is described below
commit aa92227f8d46c9c721ed1d5e73d6ed23b186cbc9
Author: MaSai <[email protected]>
AuthorDate: Fri Nov 6 17:56:44 2026 +0800
[#14674] fix(maintenance): Ignore table-level stats for partitioned
strategies (#14675)
### What changes were proposed in this pull request?
1. **#14674**: For partition-aware maintenance strategies on partitioned
tables, stop seeding trigger/score evaluation with table-level
statistics. Evaluation uses partition statistics plus shared table
metadata and policy rules only. Remove the table-context-only
short-circuit in `shouldTriggerForPartitionTable`.
2. **#14676**: `GravitinoStrategyProvider` only returns enabled policies
from `strategies()`, and `strategy(name)` rejects disabled policies.
3. **#14677**: When partition statistics are empty, skip trigger with an
explicit log; do not fall back to table-level statistics (same contract
as #14674).
4. **#14690**: Remove `System.out.println` of the full `spark-conf` map
in `builtin-iceberg-rewrite-data-files` and
`builtin-iceberg-expire-snapshots` so secrets are not written to staging
`output.log`.
### Why are the changes needed?
- `builtin-iceberg-update-stats` refreshes partition statistics for
partitioned tables and does not rewrite table-level keys. Seeding
partition evaluation with table-level statistics let stale values such
as `custom-data-file-mse` drive `submit-strategy-jobs` recommendations.
- Disabled maintenance policies should not participate in
recommendation.
- Empty partition statistics must not silently use stale table-level
metrics.
- Printing the full `spark-conf` map leaks cleartext credentials into
job staging logs.
Fixes #14674
Fixes #14676
Fixes #14677
Fixes #14690
### Does this PR introduce _any_ user-facing change?
Yes.
- Partition-aware strategies on partitioned tables decide trigger/score
from partition statistics (and metadata/rules), not from table-level
statistic values. Non-partitioned tables are unchanged.
- Disabled policies are no longer exposed as strategies.
- Rewrite/expire builtin jobs no longer print the full `spark-conf` map
to staging stdout.
- No configuration keys are added.
### How was this patch tested?
Updated and extended `TestCompactionStrategyHandler` (stale table-level
MSE regression; empty partition statistics) and
`TestGravitinoStrategyProvider` (skip/reject disabled policies). Ran:
```
./gradlew :maintenance:optimizer:test --tests
'org.apache.gravitino.maintenance.optimizer.recommender.handler.compaction.TestCompactionStrategyHandler'
--tests
'org.apache.gravitino.maintenance.optimizer.recommender.strategy.TestGravitinoStrategyProvider'
-PskipITs
```
Verified rewrite/expire jobs no longer contain the `Applied custom Spark
configurations:` println.
---------
Co-authored-by: Cursor <[email protected]>
---
.../jobs/iceberg/IcebergExpireSnapshotsJob.java | 1 -
.../jobs/iceberg/IcebergRewriteDataFilesJob.java | 1 -
.../handler/BaseExpressionStrategyHandler.java | 76 ++++++-------
.../strategy/GravitinoStrategyProvider.java | 6 +-
.../compaction/TestCompactionStrategyHandler.java | 122 +++++++++++++++++----
.../strategy/TestGravitinoStrategyProvider.java | 48 ++++++++
6 files changed, 187 insertions(+), 67 deletions(-)
diff --git
a/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergExpireSnapshotsJob.java
b/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergExpireSnapshotsJob.java
index 922f008c39..cba61e0f95 100644
---
a/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergExpireSnapshotsJob.java
+++
b/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergExpireSnapshotsJob.java
@@ -139,7 +139,6 @@ public class IcebergExpireSnapshotsJob implements
BuiltInJob {
for (Map.Entry<String, String> entry : customConfigs.entrySet()) {
sparkBuilder.config(entry.getKey(), entry.getValue());
}
- System.out.println("Applied custom Spark configurations: " +
customConfigs);
} catch (IllegalArgumentException e) {
System.err.println("Error: " + e.getMessage());
printUsage();
diff --git
a/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergRewriteDataFilesJob.java
b/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergRewriteDataFilesJob.java
index 000b4da4e2..e923f8b6e5 100644
---
a/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergRewriteDataFilesJob.java
+++
b/maintenance/jobs/src/main/java/org/apache/gravitino/maintenance/jobs/iceberg/IcebergRewriteDataFilesJob.java
@@ -148,7 +148,6 @@ public class IcebergRewriteDataFilesJob implements
BuiltInJob {
for (Map.Entry<String, String> entry : customConfigs.entrySet()) {
sparkBuilder.config(entry.getKey(), entry.getValue());
}
- System.out.println("Applied custom Spark configurations: " +
customConfigs);
} catch (IllegalArgumentException e) {
System.err.println("Error: " + e.getMessage());
printUsage();
diff --git
a/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/BaseExpressionStrategyHandler.java
b/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/BaseExpressionStrategyHandler.java
index 38230144aa..3990b161d3 100644
---
a/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/BaseExpressionStrategyHandler.java
+++
b/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/BaseExpressionStrategyHandler.java
@@ -24,7 +24,6 @@ import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.Optional;
import java.util.PriorityQueue;
import java.util.stream.Collectors;
import lombok.Value;
@@ -67,8 +66,12 @@ public abstract class BaseExpressionStrategyHandler
implements StrategyHandler {
private Map<PartitionPath, List<StatisticEntry<?>>> partitionStatistics;
private Table tableMetadata;
private NameIdentifier nameIdentifier;
- // Cached table-level context (table stats + metadata + rules), computed
once per initialize().
- private Map<String, Object> tableLevelContext;
+ /**
+ * Expression inputs from table metadata and numeric policy rules.
Table-level statistics are
+ * intentionally excluded so partition-aware strategies cannot be driven by
stale {@code
+ * statistic_meta} values after partition stats are refreshed.
+ */
+ private Map<String, Object> metadataAndRulesContext;
/** Create a handler that evaluates expressions with the default QL
evaluator. */
protected BaseExpressionStrategyHandler() {
@@ -83,7 +86,7 @@ public abstract class BaseExpressionStrategyHandler
implements StrategyHandler {
this.strategy = context.strategy();
this.tableStatistics = context.tableStatistics();
this.partitionStatistics = context.partitionStatistics();
- this.tableLevelContext = buildTableLevelContext();
+ this.metadataAndRulesContext = buildMetadataAndRulesContext();
}
@Override
@@ -135,26 +138,17 @@ public abstract class BaseExpressionStrategyHandler
implements StrategyHandler {
private boolean shouldTriggerForPartitionTable() {
if (partitionStatistics.isEmpty()) {
- LOG.info("No partition statistics available for table {}",
nameIdentifier);
+ // Partition-aware strategies require partition statistics; table-level
stats are not a
+ // fallback (see #14674 / #14677).
+ LOG.info(
+ "Skip strategy {} for partitioned table {}: no partition statistics
available",
+ strategy.name(),
+ nameIdentifier);
return false;
}
String triggerExpression = triggerExpression(strategy);
-
- // Short-circuit: try to evaluate the trigger expression with table-level
context only
- // (no partition statistics). This relies on the QL engine's left-to-right
short-circuit
- // evaluation of && operators: if a table-level predicate appears first
and evaluates to
- // false, the engine returns false without resolving subsequent
partition-level variables.
- // Note: this optimization only works when the table-level predicate is
positioned before the
- // partition-level predicate, e.g. "sort_order_count > 0 && datafile_mse <
limit" will
- // short-circuit, but "datafile_mse < limit && sort_order_count > 0" will
not.
- Optional<Boolean> evaluationResultWithoutPartitions =
- tryToEvaluateBool(triggerExpression, List.of());
- // If the trigger expression can be evaluated with table-level variables
only, return the
- // result. Otherwise, evaluate the trigger expression for each partition.
- return evaluationResultWithoutPartitions.orElseGet(
- () ->
- partitionStatistics.values().stream()
- .anyMatch(partitionStats -> evaluateBool(triggerExpression,
partitionStats)));
+ return partitionStatistics.values().stream()
+ .anyMatch(partitionStats -> evaluateBool(triggerExpression,
partitionStats));
}
private boolean shouldTriggerForNonPartitionTable() {
@@ -241,40 +235,32 @@ public abstract class BaseExpressionStrategyHandler
implements StrategyHandler {
}
}
- private Optional<Boolean> tryToEvaluateBool(
- String expression, List<StatisticEntry<?>> statistics) {
- Map<String, Object> context = buildExpressionContext(statistics);
- try {
- return expressionEvaluator.tryToEvaluateBool(expression, context);
- } catch (RuntimeException e) {
- LOG.warn("Failed to evaluate expression '{}' with context {}",
expression, context, e);
- // Per ExpressionEvaluator#tryToEvaluateBool, a failed evaluation yields
an empty Optional so
- // the caller falls back to per-partition evaluation instead of treating
it as "do not
- // trigger".
- return Optional.empty();
- }
- }
-
/**
- * Build a combined evaluation context. The {@code statistics} argument
varies per partition for
- * partitioned tables; it is layered on top of the precomputed {@link
#tableLevelContext} (table
- * statistics, table metadata, and numeric rule values) so that {@code
trigger-expr} and {@code
- * score-expr} can reference all of them.
+ * Builds the expression context for one evaluation unit.
*
- * @param statistics statistics of the unit being evaluated (a single
partition, or empty for the
- * table-level evaluation)
+ * <p>Partition-aware handlers on partitioned tables receive only {@link
#metadataAndRulesContext}
+ * plus the partition's statistics. Non-partitioned evaluation also includes
table-level
+ * statistics, applied before metadata/rules so duplicate keys keep the
historical precedence
+ * (table statistics → table metadata → numeric policy rules → unit
statistics).
+ *
+ * @param statistics statistics of the unit being evaluated (a single
partition, or empty for
+ * non-partitioned table-level evaluation)
* @return combined context
*/
private Map<String, Object> buildExpressionContext(List<StatisticEntry<?>>
statistics) {
- Map<String, Object> context = new HashMap<>(tableLevelContext);
+ Map<String, Object> context = new HashMap<>();
+ if (!isPartitionTable()) {
+ // Apply before metadata/rules so same-named properties/rules still win
(#14675 review).
+ context.putAll(StatisticsUtils.buildStatisticsContext(tableStatistics));
+ }
+ context.putAll(metadataAndRulesContext);
context.putAll(StatisticsUtils.buildStatisticsContext(statistics));
return context;
}
- /** Precompute the table-level context shared across all partition
evaluations. */
- private Map<String, Object> buildTableLevelContext() {
+ /** Precomputes table-metadata and rule inputs for expression evaluation. */
+ private Map<String, Object> buildMetadataAndRulesContext() {
Map<String, Object> context = new HashMap<>();
- context.putAll(StatisticsUtils.buildStatisticsContext(tableStatistics));
context.putAll(TableMetadataTriggerExpressionUtils.buildTableMetadataContext(tableMetadata));
context.putAll(getRulesExpressionContext(strategy));
return context;
diff --git
a/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/GravitinoStrategyProvider.java
b/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/GravitinoStrategyProvider.java
index 3f1da5c378..eebe1eb396 100644
---
a/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/GravitinoStrategyProvider.java
+++
b/maintenance/optimizer/src/main/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/GravitinoStrategyProvider.java
@@ -79,6 +79,7 @@ public class GravitinoStrategyProvider implements
StrategyProvider {
.loadTable(IdentifierUtils.removeCatalogFromIdentifier(nameIdentifier));
return Arrays.stream(t.supportsPolicies().listPolicyInfos())
.filter(Objects::nonNull)
+ .filter(Policy::enabled)
.map(this::toStrategy)
.collect(Collectors.toList());
}
@@ -89,13 +90,16 @@ public class GravitinoStrategyProvider implements
StrategyProvider {
* @param strategyName strategy name
* @return strategy
* @throws NotFoundException if the strategy does not exist
+ * @throws IllegalArgumentException if the policy exists but is disabled
*/
@Override
public Strategy strategy(String strategyName) throws NotFoundException {
Preconditions.checkArgument(
StringUtils.isNotBlank(strategyName), "strategyName must not be
blank");
try {
- return toStrategy(gravitinoClient.getPolicy(strategyName));
+ Policy policy = gravitinoClient.getPolicy(strategyName);
+ Preconditions.checkArgument(policy.enabled(), "Strategy '%s' is
disabled", strategyName);
+ return toStrategy(policy);
} catch (NoSuchPolicyException e) {
throw new NotFoundException(e, "Strategy '%s' not found", strategyName);
}
diff --git
a/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/compaction/TestCompactionStrategyHandler.java
b/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/compaction/TestCompactionStrategyHandler.java
index 2fb96959b8..212f80741e 100644
---
a/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/compaction/TestCompactionStrategyHandler.java
+++
b/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/handler/compaction/TestCompactionStrategyHandler.java
@@ -127,6 +127,34 @@ class TestCompactionStrategyHandler {
Assertions.assertFalse(lowHandler.shouldTrigger());
}
+ @Test
+ void testShouldTriggerFalseWhenPartitionStatisticsAreEmpty() {
+ NameIdentifier tableId = NameIdentifier.of("db", "table");
+ Table tableMetadata = Mockito.mock(Table.class);
+ Mockito.when(tableMetadata.partitioning())
+ .thenReturn(
+ new org.apache.gravitino.rel.expressions.transforms.Transform[] {
+ Transforms.identity("p")
+ });
+ Mockito.when(tableMetadata.sortOrder()).thenReturn(new SortOrder[0]);
+ Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
+ Mockito.when(tableMetadata.properties()).thenReturn(Map.of());
+
+ StrategyHandlerContext context =
+ StrategyHandlerContext.builder(tableId, strategy)
+ .withTableMetadata(tableMetadata)
+ .withTableStatistics(
+ List.of(new StatisticEntryImpl("datafile_mse",
StatisticValues.longValue(999999L))))
+ .withPartitionStatistics(Map.of())
+ .build();
+
+ CompactionStrategyHandler handler = new CompactionStrategyHandler();
+ handler.initialize(context);
+ Assertions.assertFalse(
+ handler.shouldTrigger(),
+ "Empty partition statistics must not trigger, even with high
table-level metrics");
+ }
+
@Test
void testShouldTriggerUsingTableMetadata() {
NameIdentifier tableId = NameIdentifier.of("db", "table");
@@ -200,7 +228,34 @@ class TestCompactionStrategyHandler {
}
@Test
- void testShouldTriggerShortCircuitsWhenTableLevelExprEvaluatesToFalse() {
+ void testNonPartitionTablePrefersMetadataOverDuplicateStatisticKey() {
+ NameIdentifier tableId = NameIdentifier.of("db", "table");
+ Strategy strategy = buildStrategyWithTriggerExpr("row_count_limit > 100");
+
+ Table tableMetadata = Mockito.mock(Table.class);
+ Mockito.when(tableMetadata.partitioning())
+ .thenReturn(new
org.apache.gravitino.rel.expressions.transforms.Transform[0]);
+ Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
+ Mockito.when(tableMetadata.sortOrder()).thenReturn(new SortOrder[0]);
+ // Metadata value satisfies the trigger; a same-named table statistic
would not.
+
Mockito.when(tableMetadata.properties()).thenReturn(Map.of("row_count_limit",
"500"));
+
+ StrategyHandlerContext context =
+ StrategyHandlerContext.builder(tableId, strategy)
+ .withTableMetadata(tableMetadata)
+ .withTableStatistics(
+ List.of(new StatisticEntryImpl("row_count_limit",
StatisticValues.longValue(1L))))
+ .build();
+
+ CompactionStrategyHandler handler = new CompactionStrategyHandler();
+ handler.initialize(context);
+ Assertions.assertTrue(
+ handler.shouldTrigger(),
+ "Non-partitioned evaluation must prefer table metadata over a
same-named table statistic");
+ }
+
+ @Test
+ void
testShouldTriggerUsesMetadataWithPartitionStatsWhenCombinedExprIsFalse() {
NameIdentifier tableId = NameIdentifier.of("db", "table");
Table tableMetadata = Mockito.mock(Table.class);
Mockito.when(tableMetadata.partitioning())
@@ -212,8 +267,6 @@ class TestCompactionStrategyHandler {
Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
Mockito.when(tableMetadata.properties()).thenReturn(Map.of());
- // Since sort_order_count == 0, QL evaluator short-circuits the && and
returns false
- // without needing to resolve the partition-level variable
Strategy strategy = buildStrategyWithTriggerExpr("sort_order_count > 0 &&
datafile_mse > 0");
Map<PartitionPath, List<StatisticEntry<?>>> partitionStats =
@@ -233,12 +286,11 @@ class TestCompactionStrategyHandler {
CompactionStrategyHandler handler = new CompactionStrategyHandler();
handler.initialize(context);
Assertions.assertFalse(
- handler.shouldTrigger(),
- "Should short-circuit to false when table-level predicate is first and
evaluates to false");
+ handler.shouldTrigger(), "Per-partition eval stays false when metadata
predicate is false");
}
@Test
- void testShouldTriggerShortCircuitsWhenTableLevelExprEvaluatesToTrue() {
+ void testShouldTriggerUsesMetadataWithPartitionStatsWhenCombinedExprIsTrue()
{
NameIdentifier tableId = NameIdentifier.of("db", "table");
Table tableMetadata = Mockito.mock(Table.class);
Mockito.when(tableMetadata.partitioning())
@@ -251,8 +303,6 @@ class TestCompactionStrategyHandler {
Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
Mockito.when(tableMetadata.properties()).thenReturn(Map.of());
- // Since sort_order_count == 1, QL evaluator short-circuits the || and
returns true
- // without needing to resolve the partition-level variable
Strategy strategy = buildStrategyWithTriggerExpr("sort_order_count > 0 ||
datafile_mse == 0");
Map<PartitionPath, List<StatisticEntry<?>>> partitionStats =
@@ -272,12 +322,11 @@ class TestCompactionStrategyHandler {
CompactionStrategyHandler handler = new CompactionStrategyHandler();
handler.initialize(context);
Assertions.assertTrue(
- handler.shouldTrigger(),
- "Should short-circuit to true when table-level predicate is first and
evaluates to true");
+ handler.shouldTrigger(), "Per-partition eval is true when metadata
predicate is true");
}
@Test
- void testShouldTriggerShortCircuitsWhenTableLevelOnlyExpr() {
+ void testShouldTriggerUsesMetadataOnlyExprAgainstEachPartition() {
NameIdentifier tableId = NameIdentifier.of("db", "table");
Table tableMetadata = Mockito.mock(Table.class);
Mockito.when(tableMetadata.partitioning())
@@ -290,7 +339,6 @@ class TestCompactionStrategyHandler {
Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
Mockito.when(tableMetadata.properties()).thenReturn(Map.of());
- // Expression uses only table-level variable, should evaluate once without
partition iteration
Strategy strategy = buildStrategyWithTriggerExpr("sort_order_count > 0");
Map<PartitionPath, List<StatisticEntry<?>>> partitionStats =
@@ -309,11 +357,51 @@ class TestCompactionStrategyHandler {
handler.initialize(context);
Assertions.assertTrue(
handler.shouldTrigger(),
- "Should short-circuit to true when table-level-only expression
evaluates to true");
+ "Metadata-only expressions still evaluate true for each partition");
+ }
+
+ @Test
+ void testShouldTriggerIgnoresStaleTableLevelStatisticsForPartitionedTable() {
+ NameIdentifier tableId = NameIdentifier.of("db", "table");
+ Table tableMetadata = Mockito.mock(Table.class);
+ Mockito.when(tableMetadata.partitioning())
+ .thenReturn(
+ new org.apache.gravitino.rel.expressions.transforms.Transform[] {
+ Transforms.identity("p")
+ });
+ Mockito.when(tableMetadata.sortOrder()).thenReturn(new SortOrder[0]);
+ Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
+ Mockito.when(tableMetadata.properties()).thenReturn(Map.of());
+
+ Strategy strategy = buildStrategyWithTriggerExpr("custom-data-file-mse >=
1000");
+
+ Map<PartitionPath, List<StatisticEntry<?>>> partitionStats =
+ Map.of(
+ PartitionPath.of(Arrays.asList(new PartitionEntryImpl("p", "1"))),
+ List.of(new StatisticEntryImpl("custom-data-file-mse",
StatisticValues.longValue(10L))),
+ PartitionPath.of(Arrays.asList(new PartitionEntryImpl("p", "2"))),
+ List.of(
+ new StatisticEntryImpl("custom-data-file-mse",
StatisticValues.longValue(20L))));
+
+ StrategyHandlerContext context =
+ StrategyHandlerContext.builder(tableId, strategy)
+ .withTableMetadata(tableMetadata)
+ .withTableStatistics(
+ List.of(
+ new StatisticEntryImpl(
+ "custom-data-file-mse",
StatisticValues.longValue(999999999L))))
+ .withPartitionStatistics(partitionStats)
+ .build();
+
+ CompactionStrategyHandler handler = new CompactionStrategyHandler();
+ handler.initialize(context);
+ Assertions.assertFalse(
+ handler.shouldTrigger(),
+ "Stale table-level MSE must not trigger when every partition is below
the threshold");
}
@Test
- void testShouldTriggerFallsBackToPartitionEvalWhenTableLevelExprIsNotFirst()
{
+ void testShouldTriggerUsesPartitionStatisticsWhenTableLevelWouldDisagree() {
NameIdentifier tableId = NameIdentifier.of("db", "table");
Table tableMetadata = Mockito.mock(Table.class);
Mockito.when(tableMetadata.partitioning())
@@ -325,8 +413,6 @@ class TestCompactionStrategyHandler {
Mockito.when(tableMetadata.columns()).thenReturn(new Column[0]);
Mockito.when(tableMetadata.properties()).thenReturn(Map.of());
- // Partition-level predicate first: datafile_mse > 0 && sort_order_count > 0
- // tryToEvaluateBool will fail on missing 'datafile_mse', so falls back to
per-partition eval
Strategy strategy = buildStrategyWithTriggerExpr("datafile_mse > 0 &&
sort_order_count > 0");
Map<PartitionPath, List<StatisticEntry<?>>> partitionStats =
@@ -343,10 +429,8 @@ class TestCompactionStrategyHandler {
CompactionStrategyHandler handler = new CompactionStrategyHandler();
handler.initialize(context);
- // sort_order_count == 0 so per-partition eval will also return false
Assertions.assertFalse(
- handler.shouldTrigger(),
- "Falls back to per-partition eval; still false because
sort_order_count is 0");
+ handler.shouldTrigger(), "Per-partition eval stays false because
sort_order_count is 0");
}
@Test
diff --git
a/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/TestGravitinoStrategyProvider.java
b/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/TestGravitinoStrategyProvider.java
index 3091fc2f87..e40eae6773 100644
---
a/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/TestGravitinoStrategyProvider.java
+++
b/maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/recommender/strategy/TestGravitinoStrategyProvider.java
@@ -55,7 +55,9 @@ class TestGravitinoStrategyProvider {
when(table.supportsPolicies()).thenReturn(supportsPolicies);
when(supportsPolicies.listPolicyInfos()).thenReturn(new Policy[] {policy1,
null, policy2});
when(policy1.name()).thenReturn("policy1");
+ when(policy1.enabled()).thenReturn(true);
when(policy2.name()).thenReturn("policy2");
+ when(policy2.enabled()).thenReturn(true);
GravitinoStrategyProvider provider = new GravitinoStrategyProvider();
FieldUtils.writeField(provider, "gravitinoClient", client, true);
@@ -68,4 +70,50 @@ class TestGravitinoStrategyProvider {
verify(supportsPolicies).listPolicyInfos();
verifyNoMoreInteractions(supportsPolicies);
}
+
+ @Test
+ void testStrategiesSkipDisabledPolicies() throws IllegalAccessException {
+ GravitinoClient client = mock(GravitinoClient.class);
+ Catalog catalog = mock(Catalog.class);
+ TableCatalog tableCatalog = mock(TableCatalog.class);
+ Table table = mock(Table.class);
+ SupportsPolicies supportsPolicies = mock(SupportsPolicies.class);
+ Policy enabled = mock(Policy.class);
+ Policy disabled = mock(Policy.class);
+ NameIdentifier identifier = NameIdentifier.of("catalog", "schema",
"table");
+
+ when(client.loadCatalog("catalog")).thenReturn(catalog);
+ when(catalog.asTableCatalog()).thenReturn(tableCatalog);
+ when(tableCatalog.loadTable(NameIdentifier.of("schema",
"table"))).thenReturn(table);
+ when(table.supportsPolicies()).thenReturn(supportsPolicies);
+ when(supportsPolicies.listPolicyInfos()).thenReturn(new Policy[] {enabled,
disabled});
+ when(enabled.name()).thenReturn("enabled");
+ when(enabled.enabled()).thenReturn(true);
+ when(disabled.name()).thenReturn("disabled");
+ when(disabled.enabled()).thenReturn(false);
+
+ GravitinoStrategyProvider provider = new GravitinoStrategyProvider();
+ FieldUtils.writeField(provider, "gravitinoClient", client, true);
+
+ List<Strategy> strategies = provider.strategies(identifier);
+
+ Assertions.assertArrayEquals(
+ new String[] {"enabled"},
strategies.stream().map(Strategy::name).toArray(String[]::new));
+ }
+
+ @Test
+ void testStrategyByNameRejectsDisabledPolicy() throws IllegalAccessException
{
+ GravitinoClient client = mock(GravitinoClient.class);
+ Policy disabled = mock(Policy.class);
+ when(client.getPolicy("disabled")).thenReturn(disabled);
+ when(disabled.enabled()).thenReturn(false);
+
+ GravitinoStrategyProvider provider = new GravitinoStrategyProvider();
+ FieldUtils.writeField(provider, "gravitinoClient", client, true);
+
+ IllegalArgumentException thrown =
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () ->
provider.strategy("disabled"));
+ Assertions.assertTrue(thrown.getMessage().contains("disabled"));
+ }
}
