Skip to content

Conversation

@R-JunmingChen
Copy link
Contributor

@R-JunmingChen R-JunmingChen commented Aug 10, 2023

Rationale for this change

As #36831 requires, a MinMax kernel for DictionaryArray is supported.

What changes are included in this PR?

Add a DictionaryType kernel for MinMax

Are these changes tested?

Yes, A test is added in aggregate_test.cc

Are there any user-facing changes?

No.

@github-actions github-actions bot added the awaiting review Awaiting review label Aug 10, 2023
@github-actions
Copy link

⚠️ GitHub issue #36831 has been automatically assigned in GitHub to PR creator.

@R-JunmingChen
Copy link
Contributor Author

R-JunmingChen commented Aug 10, 2023

Hi, reviewers, I need a guidance.

The resolved output type for DictionaryType MinMax Kernel is DictionaryType now. But I think the MinMax output type should be the DataType of the values of the DictionaryType. So, in my implementation, I use the DataType of the values of the DictionaryType as output type, which raise an error like the following img currently.
image

Can we have a correct resolved output type? and where are the codes should be changed? It should be mentioned that we can only obtain the output type in the runtime instead of compile time.

@R-JunmingChen R-JunmingChen marked this pull request as ready for review August 10, 2023 15:49
@js8544
Copy link
Collaborator

js8544 commented Aug 11, 2023

The AddMinMaxKernel function declares the signature with output MinMaxType as output, you should modify this function if you want it to output its value type instead of the dictionary itself.

Be aware that min and max also reuse the min_max kernels, so you should take a look at AddMinOrMaxAggKernel too.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 11, 2023
@R-JunmingChen R-JunmingChen requested a review from mapleFU August 14, 2023 12:24
@R-JunmingChen R-JunmingChen requested a review from pitrou November 30, 2023 01:28
@R-JunmingChen
Copy link
Contributor Author

Hi, @pitrou, @js8544. please review this PR when you have time.

@R-JunmingChen
Copy link
Contributor Author

hi, @bkietz, could you please review this PR when you have time?

Copy link
Member

@bkietz bkietz left a comment

Choose a reason for hiding this comment

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

LGTM, but please rebase and correct the CI failure

One item for potential follow up:

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 1, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 4, 2024
@R-JunmingChen R-JunmingChen requested a review from bkietz February 4, 2024 12:00
@bkietz
Copy link
Member

bkietz commented Mar 27, 2024

@R-JunmingChen sorry for letting this slip off my radar! Could you rebase please?

@R-JunmingChen
Copy link
Contributor Author

R-JunmingChen commented Apr 1, 2024

Hi, @bkietz, I have merged the latest main branch. The only one CI error seems not related

@R-JunmingChen
Copy link
Contributor Author

ping @bkietz

@R-JunmingChen
Copy link
Contributor Author

ping @bkietz again

@mapleFU
Copy link
Member

mapleFU commented Apr 23, 2024

@pitrou Would you mind check this?

@github-actions
Copy link

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

@github-actions github-actions bot added the Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise label Nov 18, 2025
@thisisnic thisisnic added Status: needs champion High impact issues which aren't being worked on but require a volunteer to move the task forward. and removed Status: stale-warning Issues and PRs flagged as stale which are due to be closed if no indication otherwise labels Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting change review Awaiting change review Component: C++ Status: needs champion High impact issues which aren't being worked on but require a volunteer to move the task forward.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] Dictionary support for ordering-based compute functions.

6 participants