-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add timestamp_micros, timestamp_millis, unix_micros, unix_millis Spark functions #9448
Conversation
✅ Deploy Preview for meta-velox canceled.
|
19111a1
to
d4ea225
Compare
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.
Basically looks good! Some small suggestions. Thanks!
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.
Thanks. Just two nits.
7f525a7
to
222335d
Compare
@mbasmanova , could you help review this PR? thanks. |
@mbasmanova Looks good to us. Can you help to take a look? |
@Yuhta , would you also please help to review this PR? Thanks |
Can we run spark expression fuzzer on the PR for 20 min? |
Spark Fuzzer is not ready yet. Rui is working on it. Currently it compares with DuckDB result. |
@FelixYBW Only aggregation fuzzer is comparing the result, scalar expression fuzzer is just checking the fast path is working correctly |
|
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Some UBSAN error:
|
d6129f9
to
2dda331
Compare
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
One more error:
|
2dda331
to
d70279c
Compare
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
d70279c
to
f549b6c
Compare
Resolved code conflict with main branch. @Yuhta could you please help trigger the Meta CI again? Thanks. |
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Some lint warnings
@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…k functions (facebookincubator#9448) Summary: Doc: https://spark.apache.org/docs/latest/api/sql/index.html#timestamp_micros https://spark.apache.org/docs/latest/api/sql/index.html#timestamp_millis https://spark.apache.org/docs/latest/api/sql/index.html#unix_micros https://spark.apache.org/docs/latest/api/sql/index.html#unix_millis Code: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala#L665 Pull Request resolved: facebookincubator#9448 Reviewed By: mbasmanova Differential Revision: D56519466 Pulled By: Yuhta fbshipit-source-id: e5b8369c72c2ce597a815e8d0d1c191d126b8345
Doc:
https://spark.apache.org/docs/latest/api/sql/index.html#timestamp_micros
https://spark.apache.org/docs/latest/api/sql/index.html#timestamp_millis
https://spark.apache.org/docs/latest/api/sql/index.html#unix_micros
https://spark.apache.org/docs/latest/api/sql/index.html#unix_millis
Code:
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala#L665