Skip to content

[SPARK-8189] [SQL] use Long for TimestampType in SQL #6733

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[SPARK-8189] [SQL] use Long for TimestampType in SQL #6733

wants to merge 6 commits into from

Conversation

@davies
Copy link
Contributor

@davies davies commented Jun 9, 2015

This PR change to use Long as internal type for TimestampType for efficiency, which means it will the precision below 100ns.

@davies
Copy link
Contributor Author

davies commented Jun 9, 2015

cc @rxin @JoshRosen

rxin
rxin reviewed Jun 9, 2015
View reviewed changes
...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
Copy link
Contributor

@rxin rxin Jun 9, 2015

Choose a reason for hiding this comment

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

this should be 0?

Copy link
Contributor Author

@davies davies Jun 10, 2015

Choose a reason for hiding this comment

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

Use -1 as an uninitialized value, easy for debug.

@rxin
Copy link
Contributor

rxin commented Jun 9, 2015

cc @liancheng to take a look at the column store stuff.

@rxin
Copy link
Contributor

rxin commented Jun 9, 2015

We also need to rewrite CastSuite to make it cover more stuff. The coverage currently is pretty low.

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34539 has finished for PR 6733 at commit 309d2e1.

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

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34573 has finished for PR 6733 at commit 246ee74.

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

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34577 has finished for PR 6733 at commit 8f77ee0.

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

@liancheng
Copy link
Contributor

liancheng commented Jun 10, 2015

In-memory columnar part looks good. The test failure was because of timestamp precision change. We can just ignore those two Hive compatibility test cases.

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34610 timed out for PR 6733 at commit 86fecfb after a configured wait of 175m.

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #895 timed out for PR 6733 at commit 86fecfb after a configured wait of 175m.

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34632 has finished for PR 6733 at commit d9565fa.

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

@rxin
Copy link
Contributor

rxin commented Jun 10, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34631 has finished for PR 6733 at commit 65cf2f1.

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

rxin
rxin reviewed Jun 10, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/json/JsonRDD.scala
Copy link
Contributor

@rxin rxin Jun 10, 2015

Choose a reason for hiding this comment

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

why is this 10000L?

Copy link
Contributor Author

@davies davies Jun 10, 2015

Choose a reason for hiding this comment

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

milliseconds to 100ns

@rxin
Copy link
Contributor

rxin commented Jun 10, 2015

I'm going to merge this, but we should fix rest of the things later.

@JoshRosen would be great if you review this again too.

@asfgit asfgit closed this in 37719e0 Jun 10, 2015
@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34638 has finished for PR 6733 at commit d9565fa.

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

@davies davies mentioned this pull request Jun 11, 2015
Closed
adrian-wang
adrian-wang reviewed Jun 11, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateUtils.scala
Copy link
Contributor

@adrian-wang adrian-wang Jun 11, 2015

Choose a reason for hiding this comment

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

since we are supporting timestamp here, Maybe we want to rename this object to DateTimeUtils, and also update the javadoc.

nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@nemccarthy
[SPARK-8189] [SQL] use Long for TimestampType in SQL
38bebde 
This PR change to use Long as internal type for TimestampType for efficiency, which means it will the precision below 100ns.

Author: Davies Liu <[email protected]>

Closes apache#6733 from davies/timestamp and squashes the following commits:

d9565fa [Davies Liu] remove print
65cf2f1 [Davies Liu] fix Timestamp in SparkR
86fecfb [Davies Liu] disable two timestamp tests
8f77ee0 [Davies Liu] fix scala style
246ee74 [Davies Liu] address comments
309d2e1 [Davies Liu] use Long for TimestampType in SQL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@davies @rxin @SparkQA @liancheng @adrian-wang