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.

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

fix(naming): add missing arg names in functions_aggregate_*.yaml #316

Merged

Conversation

vibhatha
Copy link
Contributor

@vibhatha vibhatha commented Sep 8, 2022

This PR adds the missing argument names in functions_aggregate_approx.yaml and functions_aggregate_generic.yaml.

@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 8, 2022

@jvanstraten not quite sure about the args in function

- name: "count"
    description: "Count a set of records (not field referenced)"
    impls:
      - args:
          - options: [SILENT, SATURATE, ERROR]
            required: false
        nullability: DECLARED_OUTPUT
        decomposable: MANY
        intermediate: i64
        return: i64

@vibhatha vibhatha changed the title fix: Adding missing arg names for aggregate yaml fix: Add missing arg names for aggregate yaml Sep 8, 2022
@vibhatha vibhatha changed the title fix: Add missing arg names for aggregate yaml fix: Add missing arg names in aggregate functions Sep 8, 2022
@jvanstraten
Copy link
Contributor

The sole argument there would just be named overflow. The intended behavior of the function is to count all records when aggregating (so there's no need to reference a field) if that's what you were wondering about, but I don't really like that special case either if that's what you mean... nor do I like those functions having the same name, which is something I hadn't really considered as an option yet. But I guess that's legal because the compound names are different, so... more work for the validator :/

@jvanstraten jvanstraten changed the title fix: Add missing arg names in aggregate functions fix(naming): add missing arg names in functions_aggregate_*.yaml Sep 8, 2022
@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 8, 2022

The sole argument there would just be named overflow. The intended behavior of the function is to count all records when aggregating (so there's no need to reference a field) if that's what you were wondering about, but I don't really like that special case either if that's what you mean... nor do I like those functions having the same name, which is something I hadn't really considered as an option yet. But I guess that's legal because the compound names are different, so... more work for the validator :/

So we could have the following for the unnamed count function?

- name: "count"
    description: "Count a set of records (not field referenced)"
    impls:
      - args:
          - name: overflow
          - options: [SILENT, SATURATE, ERROR]
            required: false
        nullability: DECLARED_OUTPUT
        decomposable: MANY
        intermediate: i64
        return: i64

@jvanstraten
Copy link
Contributor

Uh, more or less, but without the - before options.

@vibhatha vibhatha force-pushed the func_aggregate_approx_arg_names branch from 38ee05d to e1cf360 Compare September 9, 2022 00:28
@vibhatha
Copy link
Contributor Author

vibhatha commented Sep 9, 2022

@jvanstraten updated the PR.

extensions/functions_aggregate_generic.yaml Outdated Show resolved Hide resolved
extensions/functions_aggregate_generic.yaml Outdated Show resolved Hide resolved
@vibhatha
Copy link
Contributor Author

@jvanstraten updated the PR.

@jvanstraten jvanstraten merged commit fb92997 into substrait-io:main Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants