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.

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

Add support for DATE inputs to date extraction functions #627

Closed
wants to merge 13 commits into from
Closed

Add support for DATE inputs to date extraction functions #627

wants to merge 13 commits into from

Conversation

aditi-pandit
Copy link
Collaborator

@aditi-pandit aditi-pandit commented Nov 17, 2021 •
edited by mbasmanova
Loading

  • Add support for DATE type to date extraction functions:
    year, month, day, dow, doy, hour, minute, second, millisecond.
  • Add support for DATE type to date_trunc function.

@aditi-pandit
Add date extraction functions.
ba34ab6 
Functions added : year, month, day, dow, doy, hour,
minute, second, millisecond
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 17, 2021
@aditi-pandit aditi-pandit changed the title Add date extraction functions. Add date extraction functions Nov 17, 2021
@aditi-pandit aditi-pandit mentioned this pull request Nov 17, 2021
Closed
@mbasmanova mbasmanova changed the title Add date extraction functions Add support for DATE inputs to date extraction functions Nov 17, 2021
mbasmanova
mbasmanova reviewed Nov 17, 2021
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@aditi-pandit Looks good. Minor comments below.

velox/docs/functions/datetime.rst Outdated Show resolved Hide resolved
velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
velox/functions/prestosql/DateTimeFunctions.h Outdated Show resolved Hide resolved
@aditi-pandit
Address review comments.
99989aa 
Also add an optimization for date_trunc for Date when the unit is days.
@aditi-pandit
Copy link
Collaborator Author

@aditi-pandit Looks good. Minor comments below.

Thanks Masha. Addressed the review comments in the latest patch.

mbasmanova
mbasmanova approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@aditi-pandit Thank you, Aditi.

@mbasmanova mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 17, 2021
@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@mbasmanova
Copy link
Contributor

@aditi-pandit Aditi, the internal linter raised a few warnings. Would you take a look?

  • variable 'kSecondsInDay' defined in a header file; variable definitions in header files can lead to ODR violations
  • unused parameter 'date' in DateTimeFunctions.h:254
  • unused parameter 'config' in DateTimeFunctions.h:318

@aditi-pandit
Copy link
Collaborator Author

@aditi-pandit Aditi, the internal linter raised a few warnings. Would you take a look?

  • variable 'kSecondsInDay' defined in a header file; variable definitions in header files can lead to ODR violations
  • unused parameter 'date' in DateTimeFunctions.h:254
  • unused parameter 'config' in DateTimeFunctions.h:318

Thanks Masha. Have fixed these warnings.

@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@pedroerp
Copy link
Contributor

Super clean! I love how the new simple function framework is shaping up :) Thanks Aditi!

@mbasmanova
Copy link
Contributor

@aditi-pandit I still get this warning:

variable 'kSecondsInDay' defined in a header file; variable definitions in header files can lead to ODR violations

@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@aditi-pandit
Copy link
Collaborator Author

@aditi-pandit I still get this warning:

variable 'kSecondsInDay' defined in a header file; variable definitions in header files can lead to ODR violations

As per https://clang.llvm.org/extra/clang-tidy/checks/misc-definitions-in-headers.html making this inline should fix the violation.

Maybe we should add these flags in the invocation of clang-tidy in the format-fix scripts in velox. If you give me a list of these I could add them.

@mbasmanova
Copy link
Contributor

Maybe we should add these flags in the invocation of clang-tidy in the format-fix scripts in velox. If you give me a list of these I could add them.

Indeed. It would be nice to align linter that runs in GitHub with internal one. @mshang816 Michael, any idea how we could approach this? CC: @pedroerp

@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in a9ecd09.

@aditi-pandit aditi-pandit mentioned this pull request Nov 18, 2021
Closed
@aditi-pandit aditi-pandit deleted the extract_functions branch November 18, 2021 03:55
@aditi-pandit aditi-pandit mentioned this pull request Nov 18, 2021
Closed
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Mar 17, 2023
@zhejiangxiaomai
Quick Fix codestyle (facebookincubator#627)
0fabaae 
* fix codestyle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgpai kgpai kgpai approved these changes

@mbasmanova mbasmanova mbasmanova approved these changes

@laithsakka laithsakka Awaiting requested review from laithsakka

@kagamiori kagamiori Awaiting requested review from kagamiori

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aditi-pandit @facebook-github-bot @mbasmanova @pedroerp @kgpai