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

feat: add functions for arithmetic, rounding, logarithmic, and string transformations #245

Merged
merged 13 commits into from
Jul 25, 2022
Merged

feat: add functions for arithmetic, rounding, logarithmic, and string transformations #245

merged 13 commits into from
Jul 25, 2022

Conversation

gforsyth
Copy link
Member

Picking up on @sanjibansg 's work in #230 -- I've fixed up what I believe were the remaining comments and I've removed the round operation so that can be handled and discussed separately (although there seems to be a reasonable consensus around #230 (comment)

@richtia
Copy link
Member

richtia commented Jul 14, 2022

@gforsyth Is there a convention to follow for the name/description? Some files have both in double quotes, some only have one, some neither. Seems like it's mismatched.

@gforsyth
Copy link
Member Author

@gforsyth Is there a convention to follow for the name/description? Some files have both in double quotes, some only have one, some neither. Seems like it's mismatched.

I went with double-quotes for a single line description but no quotes for a multiline string. But that's arbitrary

@jacques-n jacques-n requested a review from jvanstraten July 15, 2022 00:25
jacques-n
jacques-n reviewed Jul 15, 2022
View reviewed changes
extensions/functions_arithmetic.yaml Outdated Show resolved Hide resolved
extensions/functions_string.yaml Outdated Show resolved Hide resolved
extensions/functions_string.yaml Outdated Show resolved Hide resolved
jvanstraten
jvanstraten requested changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@jvanstraten jvanstraten left a comment

Choose a reason for hiding this comment

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

In addition, I think you forgot to implement this comment: #230 (comment). It seems to me like we had reached consensus about that? sqrt and logarithms need domain error handling, all floating point operations need rounding mode, and SILENT/SATURATE/ERROR makes no sense for functions that return floats.

extensions/functions_arithmetic.yaml Show resolved Hide resolved
extensions/functions_arithmetic.yaml Outdated Show resolved Hide resolved
extensions/functions_rounding.yaml Outdated Show resolved Hide resolved
extensions/functions_logarithmic.yaml Outdated Show resolved Hide resolved
extensions/functions_arithmetic.yaml Show resolved Hide resolved
@gforsyth gforsyth force-pushed the gil/functions branch 3 times, most recently from 9114060 to e7696c8 Compare July 15, 2022 13:21
@gforsyth gforsyth requested a review from jvanstraten July 15, 2022 13:42
jvanstraten
jvanstraten requested changes Jul 15, 2022
View reviewed changes
extensions/functions_arithmetic.yaml Outdated Show resolved Hide resolved
extensions/functions_arithmetic.yaml Show resolved Hide resolved
extensions/functions_rounding.yaml Outdated Show resolved Hide resolved
sanjibansg and others added 11 commits July 18, 2022 08:40
@sanjibansg @gforsyth
feat: add functions for arithmetic, rounding, logarithmic, and string…
c55cff9 
… transformations

feat: sqrt returns in fp64 only

fix: ceil and floor on floating point only

feat: fixedchar in string transformations

fix: overflow on floating point negation

fix: is_nan with only floating points
@gforsyth
chore: fix yaml formatting
3ee3ba1
@gforsyth
refactor(power): promote to int64, float64, respectively
8545c39
@gforsyth
refactor(round): remove round pending further discussion
7cad2ec
@gforsyth
refactor: remove string lower/upper
895be5e 
deferring to more focused PR
@gforsyth
refactor: limit power to i64, fp64, and fp32
3b4e672 
engines should handle upromotions
@gforsyth
refactor(sqrt): remove redundant types
3f1d289
@gforsyth
refactor(logops): add rounding option
64b1a34
@gforsyth
refactor(rounding): floor and ceil return floats
18bacda
@gforsyth
chore: remove invalid args for float args
51b93ec
@gforsyth
refactor(sqrt): add rounding and domain error options
194d41b
@gforsyth gforsyth requested a review from jvanstraten July 18, 2022 12:41
jvanstraten
jvanstraten requested changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@jvanstraten jvanstraten left a comment

Choose a reason for hiding this comment

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

The logarithms still lack domain error handling (undefined for <= 0). LGTM now otherwise.

@gforsyth
Copy link
Member Author

The logarithms still lack domain error handling (undefined for <= 0). LGTM now otherwise.

Thanks for the review, @jvanstraten -- added in domain error handling, should be good now

jvanstraten
jvanstraten previously approved these changes Jul 18, 2022
View reviewed changes
@gforsyth gforsyth requested a review from jacques-n July 18, 2022 20:53
@gforsyth gforsyth requested a review from jacques-n July 19, 2022 14:22
jacques-n
jacques-n approved these changes Jul 25, 2022
View reviewed changes
Copy link
Contributor

@jacques-n jacques-n left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @gforsyth for working through the feedback!

@jacques-n jacques-n merged commit f7c5da5 into substrait-io:main Jul 25, 2022
@gforsyth gforsyth deleted the gil/functions branch July 25, 2022 19:19
@richtia richtia mentioned this pull request Jul 26, 2022
Merged
@ianmcook ianmcook mentioned this pull request Nov 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvanstraten jvanstraten jvanstraten left review comments

@jacques-n jacques-n jacques-n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gforsyth @richtia @jacques-n @jvanstraten @sanjibansg