kosiew commented on code in PR #25442:
URL: https://github.com/apache/datafusion/pull/25442#discussion_r4194298004
##########
datafusion/substrait/src/logical_plan/producer/expr/scalar_function.rs:
##########
@@ -246,35 +251,65 @@ pub fn from_like(
escape_char,
case_insensitive,
} = like;
- let function_anchor = if *case_insensitive {
- producer.register_function("ilike".to_string())
- } else {
- producer.register_function("like".to_string())
- };
// Substrait documents `output_type` as "Must be set to the return type of
// the function, exactly as derived using the declaration in the
extension",
// and a consumer that reads it rejects the call when it is unset. The type
// comes from the expression itself so that it matches what DataFusion
// derives, rather than being restated here.
let (_, output_field) = Expr::Like(like.clone()).to_field(schema)?;
let output_type = to_substrait_type_from_field(producer, &output_field)?;
+
+ make_substrait_like_expr(
+ producer,
+ *case_insensitive,
+ *negated,
+ expr,
+ pattern,
+ *escape_char,
+ schema,
+ output_type,
+ )
+}
+
+#[expect(clippy::too_many_arguments)]
+fn make_substrait_like_expr(
+ producer: &mut impl SubstraitProducer,
+ ignore_case: bool,
+ negated: bool,
Review Comment:
One optional simplification would be to keep this serialization in
`from_like`, or pass `&Like` to the helper instead of unpacking it into eight
parameters. Since the helper has one caller, that would avoid the
`too_many_arguments` suppression without changing behavior.
##########
datafusion/substrait/src/logical_plan/consumer/expr/scalar_function.rs:
##########
@@ -187,6 +187,32 @@ fn arg_list_to_binary_op_tree_inner(
}))
}
+/// Reads the `case_sensitivity` option of a `like` call.
+///
+/// Substrait says a consumer must use the first value it supports, and must
+/// reject the call when it supports none of them.
+fn case_insensitive_option(f: &ScalarFunction) -> Result<bool> {
+ let Some(option) = f
+ .options
+ .iter()
+ .find(|option| option.name.eq_ignore_ascii_case("case_sensitivity"))
+ else {
+ return Ok(false);
+ };
+ for preference in &option.preference {
+ if preference.eq_ignore_ascii_case("CASE_SENSITIVE") {
Review Comment:
Could we add table-driven tests for preference ordering and rejection cases,
including `CASE_INSENSITIVE_ASCII`, mixed-case values, and empty preference
lists? The code handles these cases, but the current roundtrip tests only cover
the single preference emitted by DataFusion, so this could also be a follow-up.
--
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]
