Skip to content

Use return_field instead of return_type for calling aggregates via FFI#17407

Merged
xudong963 merged 4 commits into
apache:mainfrom
timsaucer:bugfix/ffi-aggregate-drops-metadata
Sep 7, 2025
Merged

Use return_field instead of return_type for calling aggregates via FFI#17407
xudong963 merged 4 commits into
apache:mainfrom
timsaucer:bugfix/ffi-aggregate-drops-metadata

Conversation

@timsaucer

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

The FFI for aggregate functions was not updated to pass Fields instead of DataType when all of the other Field processing work was done. This corrects that oversight.

What changes are included in this PR?

  • Change from return_type to return_field in aggregate FFI.
  • Adds unit test for round trip metadata.
  • Update unit test to call return_field since return_type is no longer implemented.

Are these changes tested?

Yes, additional tests included.

Are there any user-facing changes?

No

@timsaucer timsaucer self-assigned this Sep 4, 2025
@github-actions github-actions Bot added the ffi Changes to the ffi crate label Sep 4, 2025

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@comphead comphead added the api change Changes the API exposed to users of the crate label Sep 4, 2025

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm thank @timsaucer
WDYT about mentioning the change in upgrading.md ?

@timsaucer

Copy link
Copy Markdown
Member Author

lgtm thank @timsaucer WDYT about mentioning the change in upgrading.md ?

I don't think there is any update anyone needs to do unless they have a unit test or something calling return_type() directly. This should be transparent to the users.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 5, 2025
@timsaucer timsaucer force-pushed the bugfix/ffi-aggregate-drops-metadata branch from f6f56bc to 4b0ac9b Compare September 5, 2025 11:02
@xudong963 xudong963 merged commit baf6f60 into apache:main Sep 7, 2025
29 checks passed
@timsaucer timsaucer deleted the bugfix/ffi-aggregate-drops-metadata branch October 13, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate documentation Improvements or additions to documentation ffi Changes to the ffi crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Aggregate function via FFI calls return_type() instead of return_field()

4 participants