-
Notifications
You must be signed in to change notification settings - Fork 208
fix date as parition key parsing issue #869
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,7 +143,7 @@ public static String convertToDeltaPartitionValue( | |
| } | ||
| if (partitionTransformType == PartitionTransformType.VALUE) { | ||
| if (fieldType == InternalType.DATE) { | ||
| return LocalDate.ofEpochDay((int) value).toString(); | ||
| return convertDatePartitionValueToString(value); | ||
| } else { | ||
| return value.toString(); | ||
| } | ||
|
|
@@ -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(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a little wary of this fallback. Also, when both parses fail this surfaces a raw Would you be open to dropping the numeric branch and throwing a domain exception on |
||
| } | ||
| } | ||
| throw new NotSupportedException( | ||
| "Unsupported DATE partition value type: " + value.getClass().getName()); | ||
| } | ||
|
|
||
| public static Object convertFromDeltaPartitionValue( | ||
| String value, | ||
| InternalType fieldType, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| // 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). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: |
||
| String deltaRepresentation = | ||
| DeltaValueConverter.convertToDeltaPartitionValue( | ||
| value, InternalType.DATE, PartitionTransformType.VALUE, ""); | ||
| assertEquals(expected, deltaRepresentation); | ||
| } | ||
|
|
||
| private static Stream<Arguments> datePartitionValues() { | ||
| return Stream.of( | ||
| Arguments.of(18181, "2019-10-12"), // Integer epoch-day (Iceberg / Delta source) | ||
| Arguments.of("2019-10-12", "2019-10-12"), // ISO date String (Paimon source) | ||
| Arguments.of("18181", "2019-10-12")); // epoch-day encoded as String | ||
| } | ||
|
|
||
| @Test | ||
| void parseWrongDateTime() throws ParseException { | ||
| String dateFormatString = "yyyy-MM-dd HH:mm:ss"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to fix this on the Paimon side rather than in the Delta sink?
From what I can tell,
PaimonPartitionExtractor.toPartitionValueswraps the rawMap<String, String>fromgeneratePartValuesinRange.scalar(...)without consulting the field'sInternalType, 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.parseValuealready does that string-to-InternalTypeswitch, 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.