-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-27344][SQL][TEST] Support the LocalDate and Instant classes in Java Bean encoders #24273
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
Changes from all commits
34231e4
506c9e7
9442afd
eeebaad
2e366b9
c71c167
c39b0d5
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 |
|---|---|---|
|
|
@@ -18,10 +18,15 @@ | |
| package test.org.apache.spark.sql; | ||
|
|
||
| import java.io.Serializable; | ||
| import java.time.Instant; | ||
| import java.time.LocalDate; | ||
| import java.util.*; | ||
|
|
||
| import org.apache.spark.sql.*; | ||
| import org.apache.spark.sql.catalyst.expressions.GenericRow; | ||
| import org.apache.spark.sql.catalyst.util.DateTimeUtils; | ||
| import org.apache.spark.sql.catalyst.util.TimestampFormatter; | ||
| import org.apache.spark.sql.internal.SQLConf; | ||
| import org.apache.spark.sql.types.DataTypes; | ||
| import org.apache.spark.sql.types.StructType; | ||
| import org.junit.*; | ||
|
|
@@ -509,4 +514,95 @@ public void setId(Integer id) { | |
| this.id = id; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testBeanWithLocalDateAndInstant() { | ||
| String originConf = spark.conf().get(SQLConf.DATETIME_JAVA8API_ENABLED().key()); | ||
| try { | ||
| spark.conf().set(SQLConf.DATETIME_JAVA8API_ENABLED().key(), "true"); | ||
| List<Row> inputRows = new ArrayList<>(); | ||
| List<LocalDateInstantRecord> expectedRecords = new ArrayList<>(); | ||
|
|
||
| for (long idx = 0 ; idx < 5 ; idx++) { | ||
| Row row = createLocalDateInstantRow(idx); | ||
| inputRows.add(row); | ||
| expectedRecords.add(createLocalDateInstantRecord(row)); | ||
| } | ||
|
|
||
| Encoder<LocalDateInstantRecord> encoder = Encoders.bean(LocalDateInstantRecord.class); | ||
|
|
||
| StructType schema = new StructType() | ||
| .add("localDateField", DataTypes.DateType) | ||
| .add("instantField", DataTypes.TimestampType); | ||
|
|
||
| Dataset<Row> dataFrame = spark.createDataFrame(inputRows, schema); | ||
| Dataset<LocalDateInstantRecord> dataset = dataFrame.as(encoder); | ||
|
|
||
| List<LocalDateInstantRecord> records = dataset.collectAsList(); | ||
|
|
||
| Assert.assertEquals(expectedRecords, records); | ||
| } finally { | ||
| spark.conf().set(SQLConf.DATETIME_JAVA8API_ENABLED().key(), originConf); | ||
| } | ||
| } | ||
|
|
||
| public static final class LocalDateInstantRecord { | ||
| private String localDateField; | ||
| private String instantField; | ||
|
|
||
| public LocalDateInstantRecord() { } | ||
|
|
||
| public String getLocalDateField() { | ||
| return localDateField; | ||
| } | ||
|
|
||
| public void setLocalDateField(String localDateField) { | ||
| this.localDateField = localDateField; | ||
| } | ||
|
|
||
| public String getInstantField() { | ||
| return instantField; | ||
| } | ||
|
|
||
| public void setInstantField(String instantField) { | ||
| this.instantField = instantField; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| LocalDateInstantRecord that = (LocalDateInstantRecord) o; | ||
| return Objects.equals(localDateField, that.localDateField) && | ||
| Objects.equals(instantField, that.instantField); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(localDateField, instantField); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
|
Member
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. Do we need to bother implementing toString here?
Member
Author
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. In regular case, |
||
| return com.google.common.base.Objects.toStringHelper(this) | ||
| .add("localDateField", localDateField) | ||
| .add("instantField", instantField) | ||
| .toString(); | ||
| } | ||
| } | ||
|
|
||
| private static Row createLocalDateInstantRow(Long index) { | ||
| Object[] values = new Object[] { LocalDate.ofEpochDay(42), Instant.ofEpochSecond(42) }; | ||
|
Member
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. Total nit, but I think you can omit "new Object[]"
Member
Author
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. You are right it can be omitted. Should I remove it?
Member
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. Not unless you're otherwise changing the code. It doesn't matter. Just a style convention I like to use |
||
| return new GenericRow(values); | ||
| } | ||
|
|
||
| private static LocalDateInstantRecord createLocalDateInstantRecord(Row recordRow) { | ||
| LocalDateInstantRecord record = new LocalDateInstantRecord(); | ||
| record.setLocalDateField(String.valueOf(recordRow.getLocalDate(0))); | ||
| Instant instant = recordRow.getInstant(1); | ||
| TimestampFormatter formatter = TimestampFormatter.getFractionFormatter( | ||
| DateTimeUtils.getZoneId(SQLConf.get().sessionLocalTimeZone())); | ||
| record.setInstantField(formatter.format(DateTimeUtils.instantToMicros(instant))); | ||
| return record; | ||
| } | ||
| } | ||
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.
Are
getLocalDateandgetInstantadded just for test? If so, seems to be overkill.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.
It was added for consistency with other supported java classes.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just checked and found you added them as external types in
RowEncoderrecently. Then it makes sense.