bryndenZh commented on PR #3500:
URL: https://github.com/apache/fluss/pull/3500#issuecomment-4845893159
> @bryndenZh Thanks for the pr.
>
> Nice catch on the root cause. One thing worth tightening before merge:
>
> **`PaimonPartitionUtils#toFlussPartitionString` is a second copy of
`PartitionUtils#convertValueOfType`.** The two `switch` statements now have to
stay in lockstep forever — if a new partition type is added on the Fluss side
and only `convertValueOfType` is updated, the lake-side and Fluss-side names
silently diverge again, which is exactly the class of bug this PR fixes. The
test that asserts "the two produce the same string" is really a symptom of
maintaining a mirror.
>
> We can delegate formatting to the single authoritative
`convertValueOfType`, and reuse the existing `PaimonRowAsFlussRow` adapter +
Fluss's `InternalRow.createFieldGetter`, so there's no per-type logic here at
all:
>
> ```java
> public static List<String> toFlussPartitionValues(
> BinaryRow partition, org.apache.fluss.types.RowType
flussPartitionType) {
> PaimonRowAsFlussRow flussRow = new
PaimonRowAsFlussRow().replaceRow(partition);
> List<String> values = new ArrayList<>(partition.getFieldCount());
> for (int i = 0; i < partition.getFieldCount(); i++) {
> DataType flussType = flussPartitionType.getTypeAt(i);
> Object value = InternalRow.createFieldGetter(flussType,
i).getFieldOrNull(flussRow);
> values.add(PartitionUtils.convertValueOfType(value,
flussType.getTypeRoot()));
> }
> return values;
> }
> ```
>
> This needs a Paimon→Fluss type converter, which doesn't exist yet (we only
have the reverse `FlussDataTypeToPaimonDataType`). Adding the symmetric
`PaimonDataTypeToFlussDataType` as a `DataTypeDefaultVisitor` is small,
mechanical, and reusable across the whole `fluss-lake-paimon` module. Watch
three mappings: Paimon `VARCHAR → STRING`, `VARBINARY → BYTES`,
`LocalZonedTimestamp → TIMESTAMP_LTZ`.
>
> A few smaller points:
>
> * **Put these in `PaimonConversions`** (the module's single Paimon↔Fluss
entry point) as `toFlussRowType` / `toFlussPartitionValues`, instead of a new
`PaimonPartitionUtils` class.
> * **`DvTableReadableSnapshotRetriever#getPartitionNameFromBinaryRow` has
the same `getString`-only bug** — point it at the same helper so both paths are
fixed together.
> * **Perf: hoist the type resolution out of the loop.** The Paimon→Fluss
conversion is identical for every split / partition bucket. Resolve the Fluss
row type once and reuse it per split:
> ```java
> RowType flussPartitionType =
toFlussRowType(fileStoreTable.schema().logicalPartitionType());
> for (Split split : scan.plan().splits()) { ...
toFlussPartitionValues(dataSplit.partition(), flussPartitionType) ... }
> ```
> * **(Separate issue) `PaimonSplitSerializer` appends the partition fields
but keeps `VERSION_1`.** An old `VERSION_1` checkpoint would hit EOF on
restore. Consider bumping to `VERSION_2` with a back-compat read path.
Thanks for the suggestions. I’ve updated the PR accordingly. PTAL.
--
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]
