minor: make HigherOrderSignature less error-prone

[gstvg]

Which issue does this PR close?

Follow up of #21679

Rationale for this change

As noted by @LiaCastaneda in #21679 (comment), the higher-order signature can be made less error prone by removing the need to set the coerce_values_for_lambdas field when coerce_values_for_lambdas should be called

What changes are included in this PR?

Remove HigherOrderSignature.coerce_values_for_lambdas/with_coerce_values_for_lambdas and modify HigherOrderUDF::coerce_values_for_lambdas return from Result<Vec<DataType>> to Result<Option<Vec<DataType>>>, and it's default implementation which now returns Ok(None) instead of an error

Are these changes tested?

Existing test cover the change

Are there any user-facing changes?

To unreleased items only

[github-actions]

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

 Cloning apache/main
Building datafusion-expr v53.1.0 (current)
Built [ 24.563s] (current)
Parsing datafusion-expr v53.1.0 (current)
Parsed [ 0.065s] (current)
Building datafusion-expr v53.1.0 (baseline)
Built [ 24.374s] (baseline)
Parsing datafusion-expr v53.1.0 (baseline)
Parsed [ 0.066s] (baseline)
Checking datafusion-expr v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 1.465s] 222 checks: 219 pass, 2 fail, 1 warn, 30 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/inherent_method_missing.ron
Failed in:
HigherOrderSignature::with_coerce_values_for_lambdas, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/5fb0c109749870b1b5b4e5eb915614403ca13208/datafusion/expr/src/higher_order_function.rs:130
--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---
Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/struct_pub_field_missing.ron
Failed in:
field coerce_values_for_lambdas of struct HigherOrderSignature, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/5fb0c109749870b1b5b4e5eb915614403ca13208/datafusion/expr/src/higher_order_function.rs:78
--- warning partial_ord_struct_fields_reordered: struct fields reordered in #[derive(PartialOrd)] struct ---
Description:
A public struct that derives PartialOrd had its fields reordered. #[derive(PartialOrd)] uses the field order to set the struct's ordering behavior, so this change may break downstream code that relies on the previous order.
ref: https://doc.rust-lang.org/std/cmp/trait.PartialOrd.html#derivable
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/partial_ord_struct_fields_reordered.ron
Failed in:
HigherOrderSignature.lambda_parameters_max_iterations moved from position 4 to 3, in /home/runner/work/datafusion/datafusion/datafusion/expr/src/higher_order_function.rs:80
Summary semver requires new major version: 2 major and 0 minor checks failed
Warning produced 1 major and 0 minor level warnings
Finished [ 51.823s] datafusion-expr

[kumarUjjawal]

Should this be updated now?

[gstvg]

Yes, thank you 030c2fb

[kosiew]

@gstvg

Thanks for the cleanup here. I did not find any blocking issues, just a couple of small doc/example updates that would make the new API shape clearer.

[kosiew]

Small docs nit: this still describes lambda output coercion as being defined by the signature, but this PR now makes coerce_values_for_lambdas opt in by returning Some(...).

Could you update this and the sibling comment above value_fields_with_higher_order_udf to mention that the method is called and may return None? That would keep the public API docs aligned with the new invariant.

[gstvg]

Thanks @kosiew, missed that one ba11072
Just to make sure, I realized the old value_fields_with_higher_order_udf docs were imprecise, it should have been "Note this does not invokes coerce_values_for_lambdaseven if requested by the function signature", so even with the new API, value_fields_with_higher_order_udf still doesn't call coerce_values_for_lambdas because it's used when lambdas output field aren't know yet

[kosiew]

This still asserts a bare Vec<DataType>, but coerce_values_for_lambdas now returns Result<Option<Vec<DataType>>>.

Could you change the expected value to Some(vec![...]) so readers copying the example see the current API shape?

[gstvg]

Also at ba11072

[kosiew]

Since there are semver breaks, let's wait for a PMC to review and if proceed, decide when to merge this.

cc @alamb

[gstvg]

Thanks @kosiew, I don't mind waiting for the review, but just to clarify, this actually changes only unreleased items, so there will be no public break (I believe the semver util checks only against main and doesn't take into account the last release)

[kosiew]

@gstvg
Good point.

Did some checking and semver bot compares against base branch main, not the latest release tag. The removed HigherOrderSignature members were introduced after 53.1.0, so this is an unreleased-main break, not a released API break.

[alamb]

@LiaCastaneda does this look good to you?
(sounds like a good change to me, but I didn't review the code)

[LiaCastaneda]

Hey, sorry for the inactivity -- I’ve been out for the last two weeks. I’ll review this today!

[LiaCastaneda]

@alamb this looks good to me, it's a small negative LOC follow up to simplify the existing api, It would be nice to include it in the next release together with #21679.
Thanks @gstvg !

[kosiew]

Looks 👍 to me

[kosiew]

The removed HigherOrderSignature members were introduced after 53.1.0, so this is an unreleased-main break, not a released API break.

No identified correctness issues.

Proceeding to merge.

[gstvg]

Thanks @kumarUjjawal@kosiew and @LiaCastaneda for the review

[kosiew]

@gstvg
Can you resolve the merge conflicts in higher_order_function.rs?

[gstvg]

Thanks for the heads-up @kosiew, It's resolved now