kosiew commented on code in PR #23148:
URL: https://github.com/apache/datafusion/pull/23148#discussion_r3489930747
##########
datafusion/ffi/src/udf/mod.rs:
##########
@@ -83,6 +83,9 @@ pub struct FFI_ScalarUDF {
/// See [`ScalarUDFImpl`] for details on short_circuits
pub short_circuits: bool,
+ /// See [`ScalarUDFImpl`] for details on is_strict.
+ pub is_strict: bool,
Review Comment:
I think this needs a compatibility guard. Adding `is_strict` as a plain
`bool` looks like it may consume existing padding in `FFI_ScalarUDF`, so the
layout can appear compatible while giving meaning to a byte that older
producers never initialized.
That means a newer consumer could read `is_strict` from an older producer
and accidentally treat the UDF as strict. If that happens,
`ForeignScalarUDF::is_strict()` could mark a non-strict UDF as strict, which
would make the new outer-join rewrite unsound.
Could we only read and propagate strictness when the FFI object is known to
come from a producer that includes this field? Otherwise I think we should
default to `false` for compatibility, or avoid propagating strictness across
FFI until the ABI can distinguish old structs.
--
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]
