czy006 commented on code in PR #4324:
URL: https://github.com/apache/amoro/pull/4324#discussion_r3804181871
##########
amoro-common/src/main/java/org/apache/amoro/config/OptimizingConfig.java:
##########
@@ -26,6 +26,8 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class OptimizingConfig {
+ public static final int DEFAULT_FRAGMENT_RATIO = 8;
Review Comment:
Agreed. I removed the default constant from `OptimizingConfig`; the
configured default remains owned by `TableProperties`.
##########
amoro-common/src/main/java/org/apache/amoro/config/OptimizingConfig.java:
##########
@@ -226,7 +228,8 @@ public OptimizingConfig setMinTargetSizeRatio(double
minTargetSizeRatio) {
}
public long maxFragmentSize() {
- return targetSize / fragmentRatio;
+ int effectiveFragmentRatio = fragmentRatio > 0 ? fragmentRatio :
DEFAULT_FRAGMENT_RATIO;
+ return targetSize / effectiveFragmentRatio;
Review Comment:
Thanks. Following the later setter-validation suggestion, the lower bound is
now applied once in `setFragmentRatio()` instead of inside `maxFragmentSize()`.
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/table/TableProperties.java:
##########
@@ -104,7 +105,8 @@ private TableProperties() {}
public static final long SELF_OPTIMIZING_MAX_TASK_SIZE_DEFAULT = 134217728;
// 128 MB
public static final String SELF_OPTIMIZING_FRAGMENT_RATIO =
"self-optimizing.fragment-ratio";
- public static final int SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT = 8;
+ public static final int SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT =
Review Comment:
Done. `SELF_OPTIMIZING_FRAGMENT_RATIO_DEFAULT` is restored to `8` in
`TableProperties`.
##########
amoro-common/src/main/java/org/apache/amoro/config/OptimizingConfig.java:
##########
@@ -226,7 +228,8 @@ public OptimizingConfig setMinTargetSizeRatio(double
minTargetSizeRatio) {
}
public long maxFragmentSize() {
- return targetSize / fragmentRatio;
+ int effectiveFragmentRatio = fragmentRatio > 0 ? fragmentRatio :
DEFAULT_FRAGMENT_RATIO;
+ return targetSize / effectiveFragmentRatio;
Review Comment:
Agreed and updated. `setFragmentRatio()` now normalizes zero and negative
values to `1`, and `maxFragmentSize()` remains a simple derived calculation.
The evaluator also reuses this method.
##########
amoro-common/src/main/java/org/apache/amoro/config/OptimizingConfig.java:
##########
@@ -226,7 +228,8 @@ public OptimizingConfig setMinTargetSizeRatio(double
minTargetSizeRatio) {
}
public long maxFragmentSize() {
- return targetSize / fragmentRatio;
+ int effectiveFragmentRatio = fragmentRatio > 0 ? fragmentRatio :
DEFAULT_FRAGMENT_RATIO;
+ return targetSize / effectiveFragmentRatio;
Review Comment:
I have not added the upper bound in this hotfix. `fragmentRatio` is
dimensionless while `targetSize` is a byte size, so this would introduce a new
cross-field invariant. Also, `TableConfigurations` currently calls
`setFragmentRatio()` before `setTargetSize()`, so the setter cannot validate
against the final target size reliably. A very large positive ratio produces a
zero fragment threshold but does not cause the division failure addressed here.
I suggest handling an upper-bound policy separately, with defined semantics and
validation after both values are available.
--
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]
