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 timestamp types as arguments #1632

Merged
merged 4 commits into from
Jun 12, 2023
Merged

feat(to_unixtime): add timestamp types as arguments #1632

merged 4 commits into from
Jun 12, 2023

Conversation

etolbakov
Copy link
Collaborator

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Add more argument types for to_unixtime function

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

#1103

@waynexia waynexia self-requested a review June 7, 2023 03:26
@killme2008
Copy link
Contributor

Is it still working? We can ship it in 0.3.1 next week.

@etolbakov
Copy link
Collaborator Author

Hey Dennis,
yes, I was catching up with @waynexia a couple of days ago regarding the question that I had.
I'm clear now and will try allocating some time during the weekend.

@etolbakov etolbakov marked this pull request as ready for review June 9, 2023 12:44
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #1632 (466c6c7) into develop (3dc45f1) will increase coverage by 0.36%.
The diff coverage is 88.48%.

❗ Current head 466c6c7 differs from pull request most recent head f4e7480. Consider uploading reports for the commit f4e7480 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1632      +/-   ##
===========================================
+ Coverage    85.78%   86.15%   +0.36%     
===========================================
  Files          563      578      +15     
  Lines        90022    93699    +3677     
===========================================
+ Hits         77228    80725    +3497     
- Misses       12794    12974     +180     

@etolbakov
Copy link
Collaborator Author

@killme2008 @waynexia
Could you please take a look when you have a spare minute?

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM, thanks @etolbakov 👍

@etolbakov
Copy link
Collaborator Author

@killme2008 @waynexia
I've addressed code review suggestions.
Thanks for pointing me out the paste crate, I found it useful.
I see that the test_alter_region_with_reopen test failed, I've double checked it works locally fine. Could it be the flaky behaviour? Would you mind just restart the action for me please?

@killme2008
Copy link
Contributor

@killme2008 @waynexia I've addressed code review suggestions. Thanks for pointing me out the paste crate, I found it useful. I see that the test_alter_region_with_reopen test failed, I've double checked it works locally fine. Could it be the flaky behaviour? Would you mind just restart the action for me please?

Great work. Looks like it's a known bug, will be fixed at #1759

@killme2008 killme2008 merged commit 51a4d66 into GreptimeTeam:develop Jun 12, 2023
@etolbakov etolbakov deleted the feat/to_unixtime-more-arguments branch June 12, 2023 09:45
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* feat(to_unixtime): add timestamp types as arguments

* feat(to_unixtime): change the return type

* feat(to_unixtime): address code review issues

* feat(to_unixtime): fix fmt issue
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