Skip to content

Conversation

@ccoffline
Copy link
Contributor

@ccoffline ccoffline commented Sep 21, 2020

Proposed changes

#4619
Add time_round functions that provides time_floor & time_ceil at each time unit.

Fix two related bugs.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • [] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] Documentation Update (if none of the other choices apply)
  • [] Code refactor (Modify the code structure, format the code, etc...)

Checklist

  • I have create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
  • Compiling and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • If this change need a document change, I have updated the document
  • Any dependent changes have been merged

Further comments

This change didn't match the style as TIMESTAMPDIFF, which has some syntactic sugar on the time units in FE. I'm not much familiar with this implementation, so I'm going to figure it out and make up for the documentation then.

@ccoffline ccoffline changed the title add time_round builtin functions [Feature] Add time_round builtin functions Sep 21, 2020
@ccoffline ccoffline marked this pull request as draft September 21, 2020 10:55
@ccoffline ccoffline marked this pull request as ready for review September 21, 2020 10:55
@morningman morningman added area/sql/function Issues or PRs related to the SQL functions good first issue kind/feature Categorizes issue or PR as related to a new feature. labels Sep 26, 2020
@morningman morningman self-assigned this Sep 26, 2020
Copy link
Contributor

@morningman morningman 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 you PR.
Could you please add document for this new function?
under sql-reference/sql-functions/date-time-functions/

@ccoffline
Copy link
Contributor Author

ccoffline commented Sep 27, 2020

Could you please add document for this new function?
under sql-reference/sql-functions/date-time-functions/

Like I said, TIMESTAMPDIFF implementation in FE has some syntactic sugar on the time units, which combines all the functions into one. This makes the doc look much prettier.

This function set has (#4619)

7(year, month, week, day, hour, minute, second) * 2(floor, ceil) * 4([timestamp], [ts, period], [ts, origin], [ts, period, origin]) = 56 functions.

I'll try to make the doc look simple.

@ccoffline
Copy link
Contributor Author

@morningman docs added

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@morningman morningman added the approved Indicates a PR has been approved by one committer. label Oct 4, 2020
@morningman morningman merged commit f3cdf16 into apache:master Oct 9, 2020
@yangzhg yangzhg mentioned this pull request Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. area/sql/function Issues or PRs related to the SQL functions good first issue kind/feature Categorizes issue or PR as related to a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants