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

[C++] Implement more "Datum" types for AggregateKernel #21519

Closed
asfimport opened this issue Mar 27, 2019 · 7 comments
Closed

[C++] Implement more "Datum" types for AggregateKernel #21519

asfimport opened this issue Mar 27, 2019 · 7 comments

Comments

@asfimport
Copy link

Currently it gives the following error if the datum isn't an array:

AggregateKernel expects Array datum

Reporter: Philipp Moritz / @pcmoritz

Note: This issue was originally created as ARROW-5022. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Francois Saint-Jacques / @fsaintjacques:
I'm not sure we want to make kernels support all datum combination. This will likely be delegated to the query engine.

@asfimport
Copy link
Author

Francois Saint-Jacques / @fsaintjacques:
Note that we can probably add "naive" support in the non-kernel but friendly api, e.g. the Sum and Mean functions.

@asfimport
Copy link
Author

Wes McKinney / @wesm:
Yes I agree that we should not proliferate kernel-specific public API endpoints and instead work toward a more generic point of entry for query execution. Otherwise we'll end up with a ton of public APIs that may take a lot of energy to maintain

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
The query engine will have to decide based on the specifics of each kernel, though. Since it's kernel-specific, perhaps it can be implemented in the kernel just as well (at least for the chunked array variant).

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
@bkietz @lidavidm What do you think the way forward should be here?

@asfimport
Copy link
Author

David Li / @lidavidm:
What is left to do here? We added scalar aggregation over scalars in ARROW-9056, and for hash aggregation, I don't think it makes sense to support anything but arrays. So going forward we should just make sure to support scalars and arrays in scalar aggregation.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
Indeed, and chunked arrays are supported as well. We can probably close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant