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 log1p function #273

Merged
merged 2 commits into from
Sep 2, 2022
Merged

feat: add log1p function #273

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e19923c
feat: add log1p function
thisisnic Aug 12, 2022
77711aa
docs: add names to log1p input args
thisisnic Sep 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions extensions/functions_logarithmic.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,5 +102,31 @@ scalar_functions:
name: "base"
description: "The logarithm base `b` to use"
return: fp64
-
name: "log1p"
description: >
Natural logarithm (base e) of 1 + x

log1p(x) => log(1+x)
cpcloud marked this conversation as resolved.
Show resolved Hide resolved
impls:
- args:
- name: rounding
options: [ TIE_TO_EVEN, TIE_AWAY_FROM_ZERO, TRUNCATE, CEILING, FLOOR ]
required: false
- name: on_domain_error
options: [ NAN, ERROR ]
required: false
- name: x
value: fp32
return: fp32
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize the surrounding functions don't do this but do these arguments need a nullability parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which arguments; as in - value: fp32? And by the same logic, would we also want a lot of the arithmetic functions to have a nullability parameter then? Or did you mean something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cpcloud why? The default is MIRROR (see table here), which is correct here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this signature handle the case of NULL inputs? It looks like it only accepts non-nullable inputs.

Copy link
Contributor

@jvanstraten jvanstraten Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, because MIRROR and DECLARED_OUTPUT are IMO weird special cases simply because the return type derivation expressions lack the generality to describe them. See https://substrait.io/expressions/scalar_functions/#nullability-handling. MIRROR and DECLARED_OUTPUT ignore the nullability flag of the arguments and accept any combination of both nullable and non-nullable arguments, and MIRROR ignores and overrides the nullability flag of the return type with the boolean or of all argument nullability flags.

Copy link
Contributor

@jvanstraten jvanstraten Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW, for the validator I'll be implementing a generalization of the type derivation grammar that uses type?? to specify "either nullable or non-nullable" in a type pattern, and type?parameter to bind the nullability to the name parameter, so variadics aside, you could describe MIRROR and DECLARED_OUTPUT more flexibly and explicitly if you want. I want to support that if only to avoid adding code for all the special cases, even if Substrait itself won't ever support that syntax.

ETA: heck, I could do away with the special cases even for variadics if I add a type?parameter? syntax for patterns, where parameter is bound to true iff any matched patterns were nullable :D But I realize that the syntax is arcane at best. Ultimately I just want to be able to round-trip the grammar with my IR for these things while being 100% compatible with Substrait grammar (for as far as that's possible at all due to all the conflicting descriptions in Substrait itself).

- args:
- name: rounding
options: [ TIE_TO_EVEN, TIE_AWAY_FROM_ZERO, TRUNCATE, CEILING, FLOOR ]
required: false
- name: on_domain_error
options: [ NAN, ERROR ]
required: false
- name: x
value: fp64
return: fp64