adamreeve commented on code in PR #55293:
URL: https://github.com/apache/arrow/pull/55293#discussion_r4544861610
##########
cpp/src/arrow/compute/expression.cc:
##########
@@ -1270,21 +1410,35 @@ struct Inequality {
auto options = checked_pointer_cast<SetLookupOptions>(is_in_call->options);
- // The maximum number of values in the is_in expression set of values
- // in order to use the simplification.
- // If the set is large there are performance implications, see:
- // https://github.com/apache/arrow/issues/46777
- constexpr int16_t kIsInSimplificationMaxValueSet = 50;
+ // Large value sets are handled by the kernel instead, see
SimplifyWithRanges.
if (options->value_set.length() > kIsInSimplificationMaxValueSet) {
return std::nullopt;
}
- const auto& lhs =
Comparison::StripOrderPreservingCasts(is_in_call->arguments[0]);
+ auto casts = OrderPreservingCasts::Strip(is_in_call->arguments[0]);
+ const Expression& lhs = *casts.inner;
if (!lhs.field_ref()) return std::nullopt;
if (*lhs.field_ref() != guarantee.target) return std::nullopt;
+ // The guarantee bounds the field but the value set is matched against the
+ // cast field.
+ auto restated = guarantee.Restate(casts);
+ if (!restated) return std::nullopt;
+ const auto& [cmp, bound] = *restated;
+
+ // Statistics based guarantees cannot bound NaN (a row group's minimum and
+ // maximum ignore it) whereas `is_in` matches NaN with NaN, so a NaN in the
+ // value set must not be filtered out. Leave such value sets alone, as the
+ // kernel does for large ones.
+ const auto value_set_type_id = options->value_set.type()->id();
+ if (value_set_type_id == Type::FLOAT || value_set_type_id == Type::DOUBLE)
{
Review Comment:
Actually I think this isn't needed. HALF_FLOAT isn't in
`IsSearchableValueSetType`, so this is probably fine.
--
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]
