[SPARK-27344][SQL][TEST] Support the LocalDate and Instant classes in Java Bean encoders #24273
Conversation
|
@dongjoon-hyun @cloud-fan Please, review this PR. |
|
Test build #104188 has finished for PR 24273 at commit
|
|
retest this please. |
| } | ||
|
|
||
| @Test | ||
| public void testSpark30() { |
There was a problem hiding this comment.
Actually it is just an unique id. I followed style of the test suite:
There was a problem hiding this comment.
I think that was the JIRA ticket ID?
There was a problem hiding this comment.
It seems it is. I can assign ticket number too like testSpark27344 or nicer name like testBeanOfLocalDateAndInstant. I would prefer the former one.
|
Test build #104189 has finished for PR 24273 at commit
|
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Do we need to bother implementing toString here?
There was a problem hiding this comment.
In regular case, toString should not be called but it is called when the assert fails.
| } | ||
|
|
||
| private static Row createLocalDateInstantRow(Long index) { | ||
| Object[] values = new Object[] { LocalDate.ofEpochDay(42), Instant.ofEpochSecond(42) }; |
There was a problem hiding this comment.
Total nit, but I think you can omit "new Object[]"
There was a problem hiding this comment.
You are right it can be omitted. Should I remove it?
There was a problem hiding this comment.
Not unless you're otherwise changing the code. It doesn't matter. Just a style convention I like to use
| * | ||
| * @throws ClassCastException when data type does not match. | ||
| */ | ||
| def getInstant(i: Int): java.time.Instant = getAs[java.time.Instant](i) |
There was a problem hiding this comment.
Are getLocalDate and getInstant added just for test? If so, seems to be overkill.
There was a problem hiding this comment.
It was added for consistency with other supported java classes.
There was a problem hiding this comment.
They are not native classes for native Spark SQL datatypes. Not necessarily to have native getters for them. If you're like to add them, I'm fine with it.
There was a problem hiding this comment.
Oh, I just checked and found you added them as external types in RowEncoder recently. Then it makes sense.
|
Test build #104211 has finished for PR 24273 at commit
|
|
thanks, merging to master! |
… Java Bean encoders ## What changes were proposed in this pull request? - Added new test for Java Bean encoder of the classes: `java.time.LocalDate` and `java.time.Instant`. - Updated comment for `Encoders.bean` - New Row getters: `getLocalDate` and `getInstant` - Extended `inferDataType` to infer types for `java.time.LocalDate` -> `DateType` and `java.time.Instant` -> `TimestampType`. ## How was this patch tested? By `JavaBeanDeserializationSuite` Closes #24273 from MaxGekk/bean-instant-localdate. Lead-authored-by: Maxim Gekk <[email protected]> Co-authored-by: Maxim Gekk <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
5 participants
What changes were proposed in this pull request?
java.time.LocalDateandjava.time.Instant.Encoders.beangetLocalDateandgetInstantinferDataTypeto infer types forjava.time.LocalDate->DateTypeandjava.time.Instant->TimestampType.How was this patch tested?
By
JavaBeanDeserializationSuite