Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
Dandandan merged PR #22106: URL: https://github.com/apache/datafusion/pull/22106 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
gstvg commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4487811929 Thanks for the heads-up @kosiew, It's resolved now -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
kosiew commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4485621615 @gstvg Can you resolve the merge conflicts in higher_order_function.rs? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
gstvg commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4480835110 Thanks @kumarUjjawal @kosiew and @LiaCastaneda for the review -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
kosiew commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4476917500 > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
LiaCastaneda commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4475607992 Hey, sorry for the inactivity -- I’ve been out for the last two weeks. I’ll review this today! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
alamb commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4460568131 @LiaCastaneda does this look good to you? (sounds like a good change to me, but I didn't review the code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
kosiew commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4456717124 @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
gstvg commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4456628244 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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
kosiew commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4456564660 Since there are semver breaks, let's wait for a PMC to review and if proceed, decide when to merge this. cc @alamb -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
gstvg commented on code in PR #22106: URL: https://github.com/apache/datafusion/pull/22106#discussion_r3240915531 ## datafusion/expr/src/type_coercion/functions.rs: ## Review Comment: Thanks @kosiew, missed that one https://github.com/apache/datafusion/pull/22106/commits/ba110728414289a3ddf2dafac31e193e850abfb5 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_lambdas` *even* 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 ## datafusion/expr/src/higher_order_function.rs: ## Review Comment: Also at https://github.com/apache/datafusion/pull/22106/commits/ba110728414289a3ddf2dafac31e193e850abfb5 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
kosiew commented on code in PR #22106: URL: https://github.com/apache/datafusion/pull/22106#discussion_r3239545490 ## datafusion/expr/src/higher_order_function.rs: ## Review Comment: This still asserts a bare `Vec`, but `coerce_values_for_lambdas` now returns `Result>>`. Could you change the expected value to `Some(vec![...])` so readers copying the example see the current API shape? ## datafusion/expr/src/type_coercion/functions.rs: ## Review Comment: 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
gstvg commented on code in PR #22106: URL: https://github.com/apache/datafusion/pull/22106#discussion_r3229887091 ## datafusion/expr/src/higher_order_function.rs: ## Review Comment: Yes, thank you https://github.com/apache/datafusion/pull/22106/commits/030c2fba2aa52ee2e4837a3e00da92f65f683b05 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
kumarUjjawal commented on code in PR #22106: URL: https://github.com/apache/datafusion/pull/22106#discussion_r3224131021 ## datafusion/expr/src/higher_order_function.rs: ## Review Comment: Should this be updated now? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] minor: make HigherOrderSignature less error-prone [datafusion]
github-actions[bot] commented on PR #22106: URL: https://github.com/apache/datafusion/pull/22106#issuecomment-4423494347 Thank you for opening this pull request! Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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 [ 28.848s] (current) Parsing datafusion-expr v53.1.0 (current) Parsed [ 0.066s] (current) Building datafusion-expr v53.1.0 (baseline) Built [ 24.749s] (baseline) Parsing datafusion-expr v53.1.0 (baseline) Parsed [ 0.068s] (baseline) Checking datafusion-expr v53.1.0 -> v53.1.0 (no change; assume patch) Checked [ 1.557s] 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/5413558180b2631b38e4baf882060ddf492fd678/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/5413558180b2631b38e4baf882060ddf492fd678/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 [ 56.779s] datafusion-expr ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
