Skip to content
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

feat(to_unixtime): add initial implementation #1186

Merged
merged 9 commits into from
Mar 21, 2023
Merged

feat(to_unixtime): add initial implementation #1186

merged 9 commits into from
Mar 21, 2023

Conversation

etolbakov
Copy link
Collaborator

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

The feature adds a new UDF to_unixtime() for conversions from a timestamp in RFC3339 format to a Unix timestamp.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#1103

@waynexia waynexia self-requested a review March 16, 2023 11:21
@etolbakov etolbakov marked this pull request as ready for review March 20, 2023 12:44
@etolbakov
Copy link
Collaborator Author

@waynexia
Could you please take a look when you have time?

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Great job! This patch generally looks good to me. Do you mind also adding some cases in our sqlness suite? Maybe a new file under common/select dir.

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #1186 (5121b10) into develop (17eb99b) will decrease coverage by 0.42%.
The diff coverage is 81.48%.

❗ Current head 5121b10 differs from pull request most recent head 3b1cb4a. Consider uploading reports for the commit 3b1cb4a to get more accurate results

@@             Coverage Diff             @@
##           develop    #1186      +/-   ##
===========================================
- Coverage    85.14%   84.72%   -0.42%     
===========================================
  Files          485      495      +10     
  Lines        71903    72593     +690     
===========================================
+ Hits         61223    61508     +285     
- Misses       10680    11085     +405     

Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

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

LGTM

@etolbakov
Copy link
Collaborator Author

@waynexia
Could you please take a look at the changes I've made based on your feedback?

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Thanks for your great work ❤️

tests/cases/standalone/common/select/dummy.sql Outdated Show resolved Hide resolved
tests/README.md Show resolved Hide resolved
@waynexia waynexia enabled auto-merge (squash) March 21, 2023 12:21
@waynexia waynexia merged commit 5397a9b into GreptimeTeam:develop Mar 21, 2023
@etolbakov etolbakov deleted the feat/to_unixtime branch March 21, 2023 12:42
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* feat(to_unixtime): add initial implementation

* feat(to_unixtime): use Timestamp for conversion

* feat(to_unixtime):  implement conversion to Result<VectorRef>

* feat(to_unixtime): make unit test pass

* feat(to_unixtime): preserve None for invalid timestamps

* feat(to_unixtime): address code review suggestions

* feat(to_unixtime): add an sqlness test

* feat(to_unixtime): adjust the assertion for the sqlness test

* Update tests/cases/standalone/common/select/dummy.sql

---------

Co-authored-by: Ruihang Xia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants