[SPARK-7199][SQL] Add date and timestamp support to UnsafeRow #5984

viirya · 2015-05-07T17:43:03Z

JIRA: https://issues.apache.org/jira/browse/SPARK-7199

SparkQA · 2015-05-07T17:50:53Z

Test build #32127 has finished for PR 5984 at commit 4c07b57.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-05-07T18:15:20Z

Test build #32129 has finished for PR 5984 at commit e42a809.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-05-07T19:11:43Z

Test build #32136 has finished for PR 5984 at commit fcf8db9.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- sealed trait LDAOptimizer
- final class EMLDAOptimizer extends LDAOptimizer
- final class OnlineLDAOptimizer extends LDAOptimizer

JoshRosen · 2015-05-08T00:02:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/rows.scala

Super minor style nit, but I think the braces here should be on separate lines, like how setDate is defined below.

SparkQA · 2015-05-08T17:16:05Z

Test build #32242 has finished for PR 5984 at commit 752251f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-05-21T08:16:30Z

@JoshRosen Any more comments or thoughts?

JoshRosen · 2015-05-21T21:34:58Z

Let me take another look in a little bit. There's been a bit of debate over whether we want to commit to supporting 128-bit timestamps in Tungsten or restrict their precision to 64 bits. The decision that we make for 1.4 may not have to be final, though, since this is an experimental feature that's off by default. Since this patch seems like a clear improvement to the current code and since we'll have the flexibility to change this later, I'm inclined to merge this for 1.4.0 and address the timestamp questions in more detail for 1.5.0.

JoshRosen · 2015-05-21T21:35:10Z

Jenkins, retest this please.

SparkQA · 2015-05-21T21:42:09Z

Test build #33287 has finished for PR 5984 at commit 752251f.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2015-05-21T21:48:30Z

Hmm, looks like DateUtils was moved or something?

marmbrus · 2015-05-21T21:55:29Z

Yes, @rxin moved it since we want to keep internal utilities out of public packages.

viirya · 2015-05-22T03:50:08Z

@JoshRosen I've updated for moved DateUtils.

SparkQA · 2015-05-22T05:47:15Z

Test build #33323 has finished for PR 5984 at commit 46946c6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2015-05-25T22:58:37Z

Jenkins, retest this please.

JoshRosen · 2015-05-25T22:59:38Z

LGTM pending Jenkins. Once the tests pass, I'll pull this in so that it doesn't become conflicted again.

SparkQA · 2015-05-26T00:58:42Z

Test build #33483 has finished for PR 5984 at commit 46946c6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-06-04T13:41:23Z

ping @JoshRosen

JoshRosen · 2015-06-08T16:54:46Z

/ping @davies, can you take a look at this? I almost merged it but was worried about causing a conflict with your code gen patch.

davies · 2015-06-08T20:15:18Z

@viirya getString/getDate/getTimestamp are public interfaces of Row, they are expensive so they should not be used internally. UnsafeRow and codegen both are internal things, so I think they don't need them at all. Could you remove these changes from this PR?

viirya · 2015-06-09T10:58:57Z

@davies But I think this PR is just intended to add date and timestamp support (setter/getter) to UnsafeRow?

davies · 2015-06-09T17:01:59Z

@viirya Yes, but we didn't need getDate/getTimestamp in catalyst, just get(ordinal) is enough for them.

SparkQA · 2015-06-13T16:31:49Z

Test build #34825 has finished for PR 5984 at commit fb532b5.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-13T18:42:31Z

Test build #34826 has finished for PR 5984 at commit 281e844.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2015-06-13T18:45:12Z

@davies, has this latest set of changes addressed your comments regarding get / set methods in codegen? Just want to make sure that we haven't overlooked that and wanted to ping you since you're more familiar me with the considerations there.

davies · 2015-06-13T20:11:14Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/UnsafeRowConverter.scala

We should use target.setInt(column, source.getInt(column))

davies · 2015-06-13T20:17:35Z

@JoshRosen Now we had separate Row and InternalRow, Row is a public interface, which has getDate/getTimestamp, but InternalRow only has Int for DateType and Long for TimestampType, InternalRow will not have getDate/setDate/getTimestamp/setTimestamp anymore. Inside catalyst, it only see Int and Long, no Date or Timestamp object.

davies · 2015-06-13T20:18:40Z

If it's still not clear yet, I could send a small PR to show how to support it in UnsafeRow.

JoshRosen · 2015-06-14T00:33:25Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeRow.java

Since timestamps are now represented as longs, we can support updates to timestamps, so we can move this into the settableFieldTypes list.

OK. I will update this later.

viirya · 2015-06-14T00:54:05Z

@davies It is very clear. Thanks.

…them.

viirya · 2015-06-16T16:13:51Z

@davies @JoshRosen I updated this. Please take a look when you are available. Thanks.

SparkQA · 2015-06-16T17:59:49Z

Test build #34993 has finished for PR 5984 at commit 672ef17.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

davies · 2015-06-17T03:30:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/UnsafeRowConverter.scala

Is it possible that we just use IntUnsafeColumnWriter for DateType? same to TimestampType

Yes. Updated now.

davies · 2015-06-17T04:34:38Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeRow.java

remove the last comma

davies · 2015-06-17T04:35:17Z

Once address the last comment, this looks good to me, thanks!

SparkQA · 2015-06-17T05:45:48Z

Test build #35018 has finished for PR 5984 at commit 0b89698.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-17T10:09:19Z

Test build #35029 has finished for PR 5984 at commit 7f21ce9.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-06-17T10:11:38Z

Looks like an unrelated test failure.

viirya · 2015-06-17T10:11:43Z

retest this please.

SparkQA · 2015-06-17T12:04:25Z

Test build #35040 has finished for PR 5984 at commit 7f21ce9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

davies · 2015-06-17T16:00:49Z

LGTM, merging this into master!

JIRA: https://issues.apache.org/jira/browse/SPARK-7199 Author: Liang-Chi Hsieh <[email protected]> Closes apache#5984 from viirya/add_date_timestamp and squashes the following commits: 7f21ce9 [Liang-Chi Hsieh] For comment. 0b89698 [Liang-Chi Hsieh] Add timestamp to settableFieldTypes. c30d490 [Liang-Chi Hsieh] Use default IntUnsafeColumnWriter and LongUnsafeColumnWriter. 672ef17 [Liang-Chi Hsieh] Remove getter/setter for Date and Timestamp and use Int and Long for them. 9f3e577 [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into add_date_timestamp 281e844 [Liang-Chi Hsieh] Fix scala style. fb532b5 [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into add_date_timestamp 80af342 [Liang-Chi Hsieh] Fix compiling error. f4f5de6 [Liang-Chi Hsieh] Fix scala style. a463e83 [Liang-Chi Hsieh] Use Long to store timestamp for rows. 635388a [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into add_date_timestamp 46946c6 [Liang-Chi Hsieh] Adapt for moved DateUtils. b16994e [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into add_date_timestamp 752251f [Liang-Chi Hsieh] Support setDate. Fix failed test. fcf8db9 [Liang-Chi Hsieh] Add functions for Date and Timestamp to SpecificRow. e42a809 [Liang-Chi Hsieh] Fix style. 4c07b57 [Liang-Chi Hsieh] Add date and timestamp support to UnsafeRow.

Add date and timestamp support to UnsafeRow.

4c07b57

Fix style.

e42a809

Add functions for Date and Timestamp to SpecificRow.

fcf8db9

JoshRosen reviewed May 8, 2015
View reviewed changes

Support setDate. Fix failed test.

752251f

viirya added 2 commits May 22, 2015 11:12

Merge remote-tracking branch 'upstream/master' into add_date_timestamp

b16994e

Adapt for moved DateUtils.

46946c6

Fix scala style.

281e844

davies reviewed Jun 13, 2015
View reviewed changes

JoshRosen reviewed Jun 14, 2015
View reviewed changes

viirya added 2 commits June 16, 2015 23:55

Merge remote-tracking branch 'upstream/master' into add_date_timestamp

9f3e577

Remove getter/setter for Date and Timestamp and use Int and Long for …

672ef17

…them.

davies reviewed Jun 17, 2015
View reviewed changes

viirya added 2 commits June 17, 2015 12:07

Use default IntUnsafeColumnWriter and LongUnsafeColumnWriter.

c30d490

Add timestamp to settableFieldTypes.

0b89698

davies reviewed Jun 17, 2015
View reviewed changes

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeRow.java Outdated

Copy link

Contributor

davies Jun 17, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the last comma

For comment.

7f21ce9

asfgit closed this in 104f30c Jun 17, 2015

viirya deleted the add_date_timestamp branch December 27, 2023 18:17

[SPARK-7199][SQL] Add date and timestamp support to UnsafeRow #5984

[SPARK-7199][SQL] Add date and timestamp support to UnsafeRow #5984

Uh oh!

Conversation

viirya commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

SparkQA commented May 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 8, 2015

Uh oh!

viirya commented May 21, 2015

Uh oh!

JoshRosen commented May 21, 2015

Uh oh!

JoshRosen commented May 21, 2015

Uh oh!

SparkQA commented May 21, 2015

Uh oh!

JoshRosen commented May 21, 2015

Uh oh!

marmbrus commented May 21, 2015

Uh oh!

viirya commented May 22, 2015

Uh oh!

SparkQA commented May 22, 2015

Uh oh!

JoshRosen commented May 25, 2015

Uh oh!

JoshRosen commented May 25, 2015

Uh oh!

SparkQA commented May 26, 2015

Uh oh!

viirya commented Jun 4, 2015

Uh oh!

JoshRosen commented Jun 8, 2015

Uh oh!

davies commented Jun 8, 2015

Uh oh!

viirya commented Jun 9, 2015

Uh oh!

davies commented Jun 9, 2015

Uh oh!

SparkQA commented Jun 13, 2015

Uh oh!

SparkQA commented Jun 13, 2015

Uh oh!

JoshRosen commented Jun 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davies commented Jun 13, 2015

Uh oh!

davies commented Jun 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Jun 14, 2015

Uh oh!

viirya commented Jun 16, 2015

Uh oh!

SparkQA commented Jun 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davies commented Jun 17, 2015

Uh oh!