fix date as parition key parsing issue - #869
Conversation
| * InternalRowPartitionComputer.generatePartValues}). This helper accepts both so the DATE | ||
| * partition case no longer fails with a {@link ClassCastException}. | ||
| */ | ||
| private static String convertDatePartitionValueToString(Object value) { |
There was a problem hiding this 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.
| try { | ||
| return LocalDate.parse(stringValue).toString(); | ||
| } catch (DateTimeParseException ex) { | ||
| return LocalDate.ofEpochDay(Long.parseLong(stringValue)).toString(); |
There was a problem hiding this 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.
| */ | ||
| @ParameterizedTest | ||
| @MethodSource("datePartitionValues") | ||
| void convertDatePartitionValueAcrossSourceRepresentations(Object value, String expected) { |
There was a problem hiding this 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.
| @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). |
There was a problem hiding this 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.
|
@yyangAtlassian Can you create a GH issues as well if possible with the exception you hit? It would be useful for the community users. |
2 participants
Important Read
PDP side test is in AppendChangelogXTableConversionIT which expose the bit and thus end up with dt as STRING to by pass it. (not a blocker)
What is the purpose of the pull request
(For example: This pull request implements the sync for delta format.)
Brief change log
(for example:)
Verify this pull request
(Please pick either of the following options)
This pull request is a trivial rework / code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)