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 population option to variance and standard deviation functions #295

Merged
merged 3 commits into from
Sep 5, 2022
Merged

feat: add population option to variance and standard deviation functions #295

merged 3 commits into from
Sep 5, 2022

Conversation

vibhatha
Copy link
Contributor

@vibhatha vibhatha commented Aug 22, 2022 •
edited by jvanstraten
Loading

In this PR population variance and standard-deviation is added.

BREAKING CHANGE: signature change of variance and std_dev

@gforsyth
Copy link
Member

hmm, I wonder if population vs sample should be options to pass to the variance and std-dev functions (with a default of sample).
I don't think they need to be separate functions.

@gforsyth
Copy link
Member

And I know postgres defaults to population for things like variance but that doesn't make very much sense to me

@vibhatha vibhatha marked this pull request as ready for review August 23, 2022 00:59
@vibhatha
Copy link
Contributor Author

@gforsyth I added an option to choose the distribution.

@jvanstraten
Copy link
Contributor

To be perfectly honest, I have no idea what the difference is, so I have no opinion on the matter. But if you believe that there's no sane default for this, you can also just make the enum required, to force the producer to pick which option they want.

@gforsyth
Copy link
Member

sample is the sane default

jvanstraten
jvanstraten previously approved these changes Aug 23, 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.

LGTM then.

@vibhatha vibhatha force-pushed the variance-std_dev-pop branch from 4d08a1b to 7e8ccd8 Compare August 26, 2022 04:55
@vibhatha
Copy link
Contributor Author

@jvanstraten any changes required?

jvanstraten
jvanstraten previously approved these changes Aug 29, 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.

I guess your merge/rebase dismissed my previous approval.

@jvanstraten jvanstraten changed the title feat: Adding population variance and standard deviation feat: add population option to variance and standard deviation functions Aug 29, 2022
@jvanstraten
Copy link
Contributor

Needs rebase due to previous merges.

@ianmcook ianmcook mentioned this pull request Sep 2, 2022
Closed
@vibhatha vibhatha force-pushed the variance-std_dev-pop branch from 7e8ccd8 to b1d7877 Compare September 5, 2022 00:14
@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 5, 2022

@jvanstraten I updated, I think the approval vanishes after pushing.

@vibhatha vibhatha requested a review from jvanstraten September 5, 2022 00:16
@jvanstraten jvanstraten merged commit c47fffa into substrait-io:main Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvanstraten jvanstraten jvanstraten 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.
3 participants
@vibhatha @gforsyth @jvanstraten