czy006 commented on code in PR #4178:
URL: https://github.com/apache/amoro/pull/4178#discussion_r3073371679
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/optimizing/plan/AbstractOptimizingEvaluator.java:
##########
@@ -275,6 +286,72 @@ public Map<Integer, Set<StructLike>> getPartitions() {
return partitions;
}
+ /**
+ * Get serializable partition paths for JSON persistence. Each entry is in
the format
+ * "specId:partitionPath", e.g. "1:dt=2024-01-01/hour=00".
+ */
+ @JsonProperty("partitionPaths")
+ public Set<String> getPartitionPaths() {
+ return partitionPaths;
+ }
+
+ @JsonProperty("partitionPaths")
+ public void setPartitionPaths(Set<String> partitionPaths) {
+ this.partitionPaths = partitionPaths != null ? partitionPaths :
Sets.newHashSet();
+ }
+
+ /**
+ * Rebuild the in-memory partitions map from serializable partition paths.
Must be called before
+ * {@link #getPartitions()} when PendingInput is deserialized from JSON.
+ *
+ * @param table the table to resolve partition specs
+ */
+ public void rebuildPartitions(MixedTable table) {
+ if (!partitions.isEmpty() || partitionPaths.isEmpty()) {
+ return;
+ }
+ for (String path : partitionPaths) {
+ try {
+ int colonIdx = path.indexOf(':');
+ if (colonIdx <= 0) {
+ continue;
+ }
+ int specId = Integer.parseInt(path.substring(0, colonIdx));
+ String partitionPath = path.substring(colonIdx + 1);
+ PartitionSpec spec =
MixedTableUtil.getMixedTablePartitionSpecById(table, specId);
+ if (spec != null) {
+ StructLike struct;
+ if (spec.isUnpartitioned() || partitionPath.isEmpty()) {
+ // For unpartitioned tables, use an empty record since there's
no path to parse
+ struct = TablePropertyUtil.EMPTY_STRUCT;
+ } else {
+ struct = DataFiles.data(spec, partitionPath);
+ }
Review Comment:
Using `org.apache.iceberg.DataFiles.data()` here will break partition
rebuild for Mixed-format tables. Iceberg's native `DataFiles.data()` delegates
to `Conversions.fromPartitionString()`, which only handles standard Iceberg
type conversions. It does **not** support Amoro's custom partition transforms:
- **`month` transform**: Mixed-format tables serialize month partitions as
`yyyy-MM` (e.g., `month=2024-01`). Iceberg's parser expects an integer month
offset, not a date string, and will throw `NumberFormatException`.
- **`hour` transform**: Similarly serialized as `yyyy-MM-dd-HH`, which
Iceberg cannot parse.
- **Hive null sentinel**: `__HIVE_DEFAULT_PARTITION__` is not handled by
Iceberg's parser.
Amoro already provides `MixedDataFiles.data(PartitionSpec, String)` in
`org.apache.amoro.utils.MixedDataFiles` (line 110) that correctly handles all
these cases via its own `fromPartitionString()`.
The `catch(Exception)` block below will silently swallow these parse
failures, logging only a warning while leaving `partitions` empty — producing
the same `Expressions.alwaysTrue()` fallback as the original bug. This
effectively makes the fix a no-op for Mixed-format tables with custom
transforms.
**Suggested fix:**
```java
import org.apache.amoro.utils.MixedDataFiles;
// ...
struct = MixedDataFiles.data(spec, partitionPath);
```
Also, please consider adding test cases for:
1. Mixed-format tables with `month`/`hour` partition transforms (round-trip
serialization)
2. Unpartitioned tables (ensure `EMPTY_STRUCT` path works correctly)
--
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]
