alamb commented on code in PR #25187:
URL: https://github.com/apache/datafusion/pull/25187#discussion_r4330011417
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -117,6 +117,10 @@ pub struct ExprSimplifier {
max_simplifier_cycles: u32,
}
+/// Largest `IN` list considered for expansion into binary comparisons.
+///
+/// The final decision also accounts for the measured performance of the column
+/// representation and whether the list values are literals.
Review Comment:
Can we please also add a link back to the PR / issue so it is easier to find
the source of the measurements?
##########
datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs:
##########
@@ -19,24 +19,47 @@
use super::THRESHOLD_INLINE_INLIST;
-use datafusion_common::Result;
+use arrow::datatypes::DataType;
use datafusion_common::tree_node::{Transformed, TreeNodeRewriter};
-use datafusion_expr::Expr;
+use datafusion_common::{DFSchema, Result};
use datafusion_expr::expr::InList;
+use datafusion_expr::{Expr, ExprSchemable};
-pub(super) struct ShortenInListSimplifier {}
+pub(super) struct ShortenInListSimplifier<'a> {
+ schema: &'a DFSchema,
+}
+
+impl<'a> ShortenInListSimplifier<'a> {
+ pub(super) fn new(schema: &'a DFSchema) -> Self {
+ Self { schema }
+ }
-impl ShortenInListSimplifier {
- pub(super) fn new() -> Self {
- Self {}
+ /// The paired `in_list_rewrite` benchmark favors membership for these
+ /// representations.
+ fn should_retain_short_inlist(&self, expr: &Expr, list: &[Expr]) -> bool {
+ let Ok(data_type) = expr.get_type(self.schema) else {
+ return false;
+ };
+ matches!(
+ data_type,
+ DataType::Int32
+ | DataType::Int64
+ | DataType::UInt64
+ | DataType::Float32
+ | DataType::Float64
+ | DataType::FixedSizeBinary(1 | 2 | 4 | 8 | 16)
+ ) && list.iter().all(|expr| {
+ expr.as_literal()
Review Comment:
when would the literals not be the same?
##########
datafusion/sqllogictest/test_files/eliminate_outer_join.slt:
##########
@@ -97,9 +97,9 @@ explain select * from t1 left join t2 on t1.a = t2.x where
t2.x in (1, 2, null);
----
logical_plan
01)Inner Join: t1.a = t2.x
-02)--Filter: t1.a = Int32(1) OR t1.a = Int32(2)
+02)--Filter: t1.a IN ([Int32(1), Int32(2), Int32(NULL)])
Review Comment:
Technically speaking this is probably a slight regression as the NULL should
be removed in the context of a filter -- but I don't think it is very
important. If we care, i think this is the right general solution
- https://github.com/apache/datafusion/issues/26542
##########
datafusion/core/benches/in_list_rewrite.rs:
##########
@@ -0,0 +1,417 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Benchmarks short SQL `IN` lists using the default optimizer. Comparing the
Review Comment:
I think it is fine -- we can just ignore it / remove in a subsequent PR
##########
datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs:
##########
@@ -19,24 +19,47 @@
use super::THRESHOLD_INLINE_INLIST;
-use datafusion_common::Result;
+use arrow::datatypes::DataType;
use datafusion_common::tree_node::{Transformed, TreeNodeRewriter};
-use datafusion_expr::Expr;
+use datafusion_common::{DFSchema, Result};
use datafusion_expr::expr::InList;
+use datafusion_expr::{Expr, ExprSchemable};
-pub(super) struct ShortenInListSimplifier {}
+pub(super) struct ShortenInListSimplifier<'a> {
+ schema: &'a DFSchema,
+}
+
+impl<'a> ShortenInListSimplifier<'a> {
+ pub(super) fn new(schema: &'a DFSchema) -> Self {
+ Self { schema }
+ }
-impl ShortenInListSimplifier {
- pub(super) fn new() -> Self {
- Self {}
+ /// The paired `in_list_rewrite` benchmark favors membership for these
Review Comment:
I think it would help to explain a little more rationale that this here
Something like
> Based on benchmarks (see `in_list_rewrite`) for these types, it is faster
to evaluate the IN list directly rather that evaluate them as an OR chain.
--
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]
