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 quantile aggregate function #279

Merged
merged 7 commits into from
Sep 6, 2022
Merged

feat: add quantile aggregate function #279

merged 7 commits into from
Sep 6, 2022

Conversation

vibhatha
Copy link
Contributor

@vibhatha vibhatha commented Aug 4, 2022

This PR includes the quantile and approx_quantile functions for aggregate functions.

@vibhatha vibhatha marked this pull request as ready for review August 4, 2022 11:37
@jvanstraten
Copy link
Contributor

From just the descriptions, implementations, and a few minutes of googling I'm not at all sure what these functions and their arguments actually do. Can you please clarify in the descriptions and include argument names?

@vibhatha vibhatha force-pushed the aggregate-quantile branch from 8663f6e to 90768f9 Compare August 23, 2022 04:14
@vibhatha
Copy link
Contributor Author

@jvanstraten I updated the PR.

@vibhatha vibhatha force-pushed the aggregate-quantile branch from 90768f9 to 18c4772 Compare August 26, 2022 05:41
@jvanstraten jvanstraten changed the title feat: adding quantile and approx_quantile functions feat: add quantile and approx_quantile functions Aug 29, 2022
jvanstraten
jvanstraten requested changes Sep 2, 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.

We discussed this function at length offline. Could you update accordingly? Right now, the description and definition just isn't good enough IMO.

@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 5, 2022

@jvanstraten I updated the PR and also for the approximate type, I created a new field called precision and added a description. Please check that description see if it requires modifications?

@vibhatha vibhatha requested a review from jvanstraten September 5, 2022 01:19
jvanstraten
jvanstraten approved these changes Sep 5, 2022
View reviewed changes
Copy link
Contributor

@jvanstraten jvanstraten left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM now!

jvanstraten
jvanstraten requested changes Sep 5, 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.

Actually, correction (sorry), the same comments I gave on the other PR for precision apply here. I guess for approximate the list items should at least be monotonously increasing/decreasing if the input is sorted ascending/descending? Again, I'm also fine with just omitting its description for now since it will be a standard option.

@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 5, 2022

Actually, correction (sorry), the same comments I gave on the other PR for precision apply here.

I updated the description of the precision.

I guess for approximate the list items should at least be monotonously increasing/decreasing if the input is sorted ascending/descending? Again, I'm also fine with just omitting its description for now since it will be a standard option.

Shall we include options about the ordering too? If so, it shouldn't be an option of precision but the operator itself, right?

jvanstraten
jvanstraten requested changes Sep 5, 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.

Shall we include options about the ordering too? If so, it shouldn't be an option of precision but the operator itself, right?

No, because aggregate function bindings already include specialized options for sorting prior to aggregation in protobuf land.

Also needs a rebase.

extensions/functions_arithmetic.yaml Outdated
Comment on lines 1117 to 1118
- EXACT: provides the highest accurate output
- APPROXIMATE: provides a sub-optimal output
Copy link
Contributor

Choose a reason for hiding this comment

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

I updated the description of the precision.

I mean here. Or maybe you forgot to push?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay now?

@jvanstraten
Copy link
Contributor

We also need to add ordered: true to all these implementations. I didn't realize that wasn't the default behavior. See also #312

@vibhatha vibhatha requested a review from jvanstraten September 6, 2022 01:02
@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 6, 2022

We also need to add ordered: true to all these implementations. I didn't realize that wasn't the default behavior. See also #312

@jvanstraten updated.

jvanstraten
jvanstraten approved these changes Sep 6, 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 think this looks good now.

@jvanstraten jvanstraten changed the title feat: add quantile and approx_quantile functions feat: add quantile aggregate function Sep 6, 2022
@jvanstraten jvanstraten merged commit de6bc9f into substrait-io:main Sep 6, 2022
@ianmcook ianmcook mentioned this pull request Sep 6, 2022
Closed
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.
2 participants
@vibhatha @jvanstraten