vinishjail97 commented on code in PR #869:
URL: https://github.com/apache/incubator-xtable/pull/869#discussion_r3732297202
##########
xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaValueConverter.java:
##########
@@ -74,6 +74,34 @@ public void formattedValueDifferentTypesForPartition(
assertEquals(fieldValue, internalRepresentation);
}
+ /**
+ * Reproduces the Paimon-source -> Delta-target DATE partition failure.
+ *
+ * <p>{@link
org.apache.xtable.paimon.PaimonPartitionExtractor#toPartitionValues} always
emits
+ * partition values as {@link String} (via {@code
InternalRowPartitionComputer.generatePartValues},
+ * e.g. {@code "2019-10-12"}). When such a value reaches {@code
convertToDeltaPartitionValue} for a
+ * DATE partition field with a VALUE transform, the current code executes
{@code (int) value} on a
+ * String, throwing a {@link ClassCastException}. DATE as a regular
(non-partition) column works
+ * because it flows through the column-stat path with a real epoch-day int.
+ */
+ @ParameterizedTest
+ @MethodSource("datePartitionValues")
+ void convertDatePartitionValueAcrossSourceRepresentations(Object value,
String expected) {
Review Comment:
Thanks for adding coverage here. My worry is that testing the converter in
isolation would not fail if the source-side representation is the underlying
issue, and it would not reach the Paimon -> Iceberg path.
Would it be possible to also add a DATE case to
`TestPaimonPartitionExtractor` (it only covers a STRING key today), and an
end-to-end Paimon -> Delta case with a DATE partition column? I noticed the
description mentions the PDP-side test declares `dt` as STRING to work around
this, which seems like the case most worth pinning down.
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaValueConverter.java:
##########
@@ -154,6 +154,35 @@ public static String convertToDeltaPartitionValue(
}
}
+ /**
+ * Serializes a DATE partition value to the canonical {@code yyyy-MM-dd}
string used in the Delta
+ * partition path/log.
+ *
+ * <p>Different conversion sources surface DATE partition values in
different runtime forms: the
+ * Iceberg and Delta sources provide an {@link Integer} epoch-day, whereas
the Paimon source
+ * provides an already-formatted {@code yyyy-MM-dd} {@link String} (see
{@code
+ * PaimonPartitionExtractor#toPartitionValues}, which derives values from
{@code
+ * InternalRowPartitionComputer.generatePartValues}). This helper accepts
both so the DATE
+ * partition case no longer fails with a {@link ClassCastException}.
+ */
+ private static String convertDatePartitionValueToString(Object value) {
Review Comment:
Would it make sense to fix this on the Paimon side rather than in the Delta
sink?
From what I can tell, `PaimonPartitionExtractor.toPartitionValues` wraps the
raw `Map<String, String>` from `generatePartValues` in `Range.scalar(...)`
without consulting the field's `InternalType`, so values arrive as Strings for
every type -- DATE just happens to be the one that fails loudly. If that
reading is right, Paimon -> Iceberg with a DATE partition and any INT / LONG /
BOOLEAN partition key would still be wrong after this change, only quietly.
`PathBasedPartitionValuesExtractor.parseValue` already does that
string-to-`InternalType` switch, so perhaps it could be shared? I may well be
missing a reason the conversion has to happen at the sink -- if so, a note in
the description would help.
##########
xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaValueConverter.java:
##########
@@ -74,6 +74,34 @@ public void formattedValueDifferentTypesForPartition(
assertEquals(fieldValue, internalRepresentation);
}
+ /**
+ * Reproduces the Paimon-source -> Delta-target DATE partition failure.
+ *
+ * <p>{@link
org.apache.xtable.paimon.PaimonPartitionExtractor#toPartitionValues} always
emits
+ * partition values as {@link String} (via {@code
InternalRowPartitionComputer.generatePartValues},
+ * e.g. {@code "2019-10-12"}). When such a value reaches {@code
convertToDeltaPartitionValue} for a
+ * DATE partition field with a VALUE transform, the current code executes
{@code (int) value} on a
+ * String, throwing a {@link ClassCastException}. DATE as a regular
(non-partition) column works
+ * because it flows through the column-stat path with a real epoch-day int.
+ */
+ @ParameterizedTest
+ @MethodSource("datePartitionValues")
+ void convertDatePartitionValueAcrossSourceRepresentations(Object value,
String expected) {
+ // Epoch day 18181 == "2019-10-12". Integer form is produced by the
Iceberg/Delta sources; the
+ // String form is produced by the Paimon source
(InternalRowPartitionComputer.generatePartValues).
Review Comment:
Minor: `spotless:check` looks like it fails on this file -- lines 81, 82 and
91 are over the 100-column limit. I ran `mvn -pl xtable-core spotless:check`
locally to confirm. `mvn spotless:apply` should sort it out.
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaValueConverter.java:
##########
@@ -154,6 +154,35 @@ public static String convertToDeltaPartitionValue(
}
}
+ /**
+ * Serializes a DATE partition value to the canonical {@code yyyy-MM-dd}
string used in the Delta
+ * partition path/log.
+ *
+ * <p>Different conversion sources surface DATE partition values in
different runtime forms: the
+ * Iceberg and Delta sources provide an {@link Integer} epoch-day, whereas
the Paimon source
+ * provides an already-formatted {@code yyyy-MM-dd} {@link String} (see
{@code
+ * PaimonPartitionExtractor#toPartitionValues}, which derives values from
{@code
+ * InternalRowPartitionComputer.generatePartValues}). This helper accepts
both so the DATE
+ * partition case no longer fails with a {@link ClassCastException}.
+ */
+ private static String convertDatePartitionValueToString(Object value) {
+ if (value instanceof Number) {
+ return LocalDate.ofEpochDay(((Number) value).longValue()).toString();
+ }
+ if (value instanceof String) {
+ // Already an ISO-8601 date; parse to validate and normalize (also
tolerates an epoch-day
+ // encoded as a string).
+ String stringValue = ((String) value).trim();
+ try {
+ return LocalDate.parse(stringValue).toString();
+ } catch (DateTimeParseException ex) {
+ return LocalDate.ofEpochDay(Long.parseLong(stringValue)).toString();
Review Comment:
I am a little wary of this fallback. `"2019"` would parse as epoch-day 2019
and be written as `1975-07-14`, and `"20191012"` as year 57786 -- a wrong
partition value in the Delta log rather than an error, which is hard to spot
later.
Also, when both parses fail this surfaces a raw `NumberFormatException`
rather than the `NotSupportedException` used just below. I believe Paimon's
`__DEFAULT_PARTITION__` null-partition sentinel would reach that path.
Would you be open to dropping the numeric branch and throwing a domain
exception on `DateTimeParseException`, plus handling the null sentinel
explicitly? Hudi's path-based extractor maps `__HIVE_DEFAULT_PARTITION__` to
null, so there may be a pattern worth following.
--
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]
