adamreeve commented on code in PR #55171:
URL: https://github.com/apache/arrow/pull/55171#discussion_r4419644448
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -1245,6 +1255,198 @@ TEST(TestBinaryViewCompareKernel, ArrayScalar) {
}
}
+TEST(TestBinaryViewCompareKernel, MixedStringTypes) {
+ // Array - Array (short inline strings <= 12 bytes)
+ auto str_arr = ArrayFromJSON(utf8(), R"(["a", "b", null, "d"])");
+ auto large_str_arr = ArrayFromJSON(large_utf8(), R"(["a", "b", null, "d"])");
+ auto view_arr = ArrayFromJSON(utf8_view(), R"(["a", "x", "c", null])");
+
+ CheckScalarBinary("equal", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", view_arr, str_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", large_str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", view_arr, large_str_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+
+ CheckScalarBinary("not_equal", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[false, true, null, null]"));
+ CheckScalarBinary("less", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[false, true, null, null]"));
+ CheckScalarBinary("greater", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[false, false, null, null]"));
+
+ // Array - Array (long strings > 12 bytes testing out-of-line prefix +
buffer lookup)
+ auto long_str_arr = ArrayFromJSON(
+ utf8(),
+ R"(["this_is_a_long_string_1", "this_is_a_long_string_2", null,
"short"])");
+ auto long_view_arr = ArrayFromJSON(
+ utf8_view(),
+ R"(["this_is_a_long_string_1", "this_is_a_different_long_string", "c",
null])");
+ CheckScalarBinary("equal", long_str_arr, long_view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", long_view_arr, long_str_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("not_equal", long_str_arr, long_view_arr,
+ ArrayFromJSON(boolean(), "[false, true, null, null]"));
+
+ // Empty arrays
+ auto empty_str = ArrayFromJSON(utf8(), "[]");
+ auto empty_view = ArrayFromJSON(utf8_view(), "[]");
+ CheckScalarBinary("equal", empty_str, empty_view, ArrayFromJSON(boolean(),
"[]"));
+ CheckScalarBinary("equal", empty_view, empty_str, ArrayFromJSON(boolean(),
"[]"));
+
+ // All-null arrays
+ auto null_str = ArrayFromJSON(utf8(), "[null, null]");
+ auto null_view = ArrayFromJSON(utf8_view(), "[null, null]");
+ CheckScalarBinary("equal", null_str, null_view,
+ ArrayFromJSON(boolean(), "[null, null]"));
+
+ // Sliced arrays
+ auto sliced_str = str_arr->Slice(1, 2);
+ auto sliced_view = view_arr->Slice(1, 2);
+ CheckScalarBinary("equal", sliced_str, sliced_view,
+ ArrayFromJSON(boolean(), "[false, null]"));
+ CheckScalarBinary("equal", sliced_view, sliced_str,
+ ArrayFromJSON(boolean(), "[false, null]"));
+
+ // Array - Scalar
+ auto scalar_str = ScalarFromJSON(utf8(), R"("b")");
+ auto scalar_view = ScalarFromJSON(utf8_view(), R"("b")");
+ auto scalar_large = ScalarFromJSON(large_utf8(), R"("b")");
+
+ auto arr_view = ArrayFromJSON(utf8_view(), R"(["a", "b", "c", null])");
+ auto arr_str = ArrayFromJSON(utf8(), R"(["a", "b", "c", null])");
+
+ CheckScalarBinary("equal", arr_view, scalar_str,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", scalar_str, arr_view,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", arr_str, scalar_view,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", scalar_view, arr_str,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", arr_view, scalar_large,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+
+ // Long scalar (> 12 bytes)
+ auto long_scalar_str = ScalarFromJSON(utf8(),
R"("this_is_a_long_string_1")");
+ auto long_scalar_view = ScalarFromJSON(utf8_view(),
R"("this_is_a_long_string_1")");
+ CheckScalarBinary("equal", long_view_arr, long_scalar_str,
+ ArrayFromJSON(boolean(), "[true, false, false, null]"));
+ CheckScalarBinary("equal", long_scalar_str, long_view_arr,
+ ArrayFromJSON(boolean(), "[true, false, false, null]"));
+ CheckScalarBinary("equal", long_str_arr, long_scalar_view,
+ ArrayFromJSON(boolean(), "[true, false, null, false]"));
+
+ // Asymmetric slice offsets
+ auto sliced_lhs = str_arr->Slice(1, 2); // ["b", null]
+ auto sliced_rhs = view_arr->Slice(2, 2); // ["c", null]
+ CheckScalarBinary("equal", sliced_lhs, sliced_rhs,
+ ArrayFromJSON(boolean(), "[false, null]"));
+
+ // Null scalar comparisons
+ auto null_scalar_str = MakeNullScalar(utf8());
+ auto null_scalar_view = MakeNullScalar(utf8_view());
+ CheckScalarBinary("equal", view_arr, null_scalar_str,
+ ArrayFromJSON(boolean(), "[null, null, null, null]"));
+ CheckScalarBinary("equal", str_arr, null_scalar_view,
+ ArrayFromJSON(boolean(), "[null, null, null, null]"));
+
+ // Embedded null bytes (\0) across inline (<=12B) and out-of-line (>12B)
views
+ auto null_byte_str = ArrayFromJSON(
+ utf8(), R"(["a\u0000b", "a\u0000x", "long_prefix_\u0000_x"])");
+ auto null_byte_view = ArrayFromJSON(
+ utf8_view(), R"(["a\u0000b", "a\u0000y", "long_prefix_\u0000_y"])");
+ CheckScalarBinary("equal", null_byte_str, null_byte_view,
+ ArrayFromJSON(boolean(), "[true, false, false]"));
+ CheckScalarBinary("less", null_byte_str, null_byte_view,
+ ArrayFromJSON(boolean(), "[false, true, true]"));
+
+ // Binary and BinaryView
+ auto bin_arr = ArrayFromJSON(binary(), R"(["a", "b", null, "d"])");
+ auto bin_view_arr = ArrayFromJSON(binary_view(), R"(["a", "x", "c", null])");
+ CheckScalarBinary("equal", bin_arr, bin_view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", bin_view_arr, bin_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+}
+
+TEST(TestCompareKernel, TestIssue54668) {
+ // Direct mixed comparison between utf8 / large_utf8 and utf8_view without
promotion
+ for (std::string name :
+ {"equal", "not_equal", "less", "less_equal", "greater",
"greater_equal"}) {
+ CheckDispatchBest(name, {utf8(), utf8_view()}, {utf8(), utf8_view()});
+ CheckDispatchBest(name, {utf8_view(), utf8()}, {utf8_view(), utf8()});
+ CheckDispatchBest(name, {large_utf8(), utf8_view()}, {large_utf8(),
utf8_view()});
+ CheckDispatchBest(name, {utf8_view(), large_utf8()}, {utf8_view(),
large_utf8()});
+ }
Review Comment:
These checks are redundant, they duplicate the additions in
`TestCompareKernel::DispatchBest` so could be removed here.
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -1245,6 +1255,198 @@ TEST(TestBinaryViewCompareKernel, ArrayScalar) {
}
}
+TEST(TestBinaryViewCompareKernel, MixedStringTypes) {
+ // Array - Array (short inline strings <= 12 bytes)
+ auto str_arr = ArrayFromJSON(utf8(), R"(["a", "b", null, "d"])");
+ auto large_str_arr = ArrayFromJSON(large_utf8(), R"(["a", "b", null, "d"])");
+ auto view_arr = ArrayFromJSON(utf8_view(), R"(["a", "x", "c", null])");
+
+ CheckScalarBinary("equal", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", view_arr, str_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", large_str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", view_arr, large_str_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+
+ CheckScalarBinary("not_equal", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[false, true, null, null]"));
+ CheckScalarBinary("less", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[false, true, null, null]"));
+ CheckScalarBinary("greater", str_arr, view_arr,
+ ArrayFromJSON(boolean(), "[false, false, null, null]"));
+
+ // Array - Array (long strings > 12 bytes testing out-of-line prefix +
buffer lookup)
+ auto long_str_arr = ArrayFromJSON(
+ utf8(),
+ R"(["this_is_a_long_string_1", "this_is_a_long_string_2", null,
"short"])");
+ auto long_view_arr = ArrayFromJSON(
+ utf8_view(),
+ R"(["this_is_a_long_string_1", "this_is_a_different_long_string", "c",
null])");
+ CheckScalarBinary("equal", long_str_arr, long_view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", long_view_arr, long_str_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("not_equal", long_str_arr, long_view_arr,
+ ArrayFromJSON(boolean(), "[false, true, null, null]"));
+
+ // Empty arrays
+ auto empty_str = ArrayFromJSON(utf8(), "[]");
+ auto empty_view = ArrayFromJSON(utf8_view(), "[]");
+ CheckScalarBinary("equal", empty_str, empty_view, ArrayFromJSON(boolean(),
"[]"));
+ CheckScalarBinary("equal", empty_view, empty_str, ArrayFromJSON(boolean(),
"[]"));
+
+ // All-null arrays
+ auto null_str = ArrayFromJSON(utf8(), "[null, null]");
+ auto null_view = ArrayFromJSON(utf8_view(), "[null, null]");
+ CheckScalarBinary("equal", null_str, null_view,
+ ArrayFromJSON(boolean(), "[null, null]"));
+
+ // Sliced arrays
+ auto sliced_str = str_arr->Slice(1, 2);
+ auto sliced_view = view_arr->Slice(1, 2);
+ CheckScalarBinary("equal", sliced_str, sliced_view,
+ ArrayFromJSON(boolean(), "[false, null]"));
+ CheckScalarBinary("equal", sliced_view, sliced_str,
+ ArrayFromJSON(boolean(), "[false, null]"));
+
+ // Array - Scalar
+ auto scalar_str = ScalarFromJSON(utf8(), R"("b")");
+ auto scalar_view = ScalarFromJSON(utf8_view(), R"("b")");
+ auto scalar_large = ScalarFromJSON(large_utf8(), R"("b")");
+
+ auto arr_view = ArrayFromJSON(utf8_view(), R"(["a", "b", "c", null])");
+ auto arr_str = ArrayFromJSON(utf8(), R"(["a", "b", "c", null])");
+
+ CheckScalarBinary("equal", arr_view, scalar_str,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", scalar_str, arr_view,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", arr_str, scalar_view,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", scalar_view, arr_str,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+ CheckScalarBinary("equal", arr_view, scalar_large,
+ ArrayFromJSON(boolean(), "[false, true, false, null]"));
+
+ // Long scalar (> 12 bytes)
+ auto long_scalar_str = ScalarFromJSON(utf8(),
R"("this_is_a_long_string_1")");
+ auto long_scalar_view = ScalarFromJSON(utf8_view(),
R"("this_is_a_long_string_1")");
+ CheckScalarBinary("equal", long_view_arr, long_scalar_str,
+ ArrayFromJSON(boolean(), "[true, false, false, null]"));
+ CheckScalarBinary("equal", long_scalar_str, long_view_arr,
+ ArrayFromJSON(boolean(), "[true, false, false, null]"));
+ CheckScalarBinary("equal", long_str_arr, long_scalar_view,
+ ArrayFromJSON(boolean(), "[true, false, null, false]"));
+
+ // Asymmetric slice offsets
+ auto sliced_lhs = str_arr->Slice(1, 2); // ["b", null]
+ auto sliced_rhs = view_arr->Slice(2, 2); // ["c", null]
+ CheckScalarBinary("equal", sliced_lhs, sliced_rhs,
+ ArrayFromJSON(boolean(), "[false, null]"));
+
+ // Null scalar comparisons
+ auto null_scalar_str = MakeNullScalar(utf8());
+ auto null_scalar_view = MakeNullScalar(utf8_view());
+ CheckScalarBinary("equal", view_arr, null_scalar_str,
+ ArrayFromJSON(boolean(), "[null, null, null, null]"));
+ CheckScalarBinary("equal", str_arr, null_scalar_view,
+ ArrayFromJSON(boolean(), "[null, null, null, null]"));
+
+ // Embedded null bytes (\0) across inline (<=12B) and out-of-line (>12B)
views
+ auto null_byte_str = ArrayFromJSON(
+ utf8(), R"(["a\u0000b", "a\u0000x", "long_prefix_\u0000_x"])");
+ auto null_byte_view = ArrayFromJSON(
+ utf8_view(), R"(["a\u0000b", "a\u0000y", "long_prefix_\u0000_y"])");
+ CheckScalarBinary("equal", null_byte_str, null_byte_view,
+ ArrayFromJSON(boolean(), "[true, false, false]"));
+ CheckScalarBinary("less", null_byte_str, null_byte_view,
+ ArrayFromJSON(boolean(), "[false, true, true]"));
+
+ // Binary and BinaryView
+ auto bin_arr = ArrayFromJSON(binary(), R"(["a", "b", null, "d"])");
+ auto bin_view_arr = ArrayFromJSON(binary_view(), R"(["a", "x", "c", null])");
+ CheckScalarBinary("equal", bin_arr, bin_view_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+ CheckScalarBinary("equal", bin_view_arr, bin_arr,
+ ArrayFromJSON(boolean(), "[true, false, null, null]"));
+}
+
+TEST(TestCompareKernel, TestIssue54668) {
Review Comment:
This test seems to cover an odd mix of things. Can the string comparisons
please be moved into the `MixedStringTypes` test, and this test renamed to
`MixedDecimalScaleTests` and only include the decimal tests?
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -478,6 +499,11 @@ std::shared_ptr<ScalarFunction>
MakeFlippedCompare(std::string name,
std::make_shared<CompareFunction>(name, Arity::Binary(), std::move(doc));
for (const ScalarKernel* kernel : func.kernels()) {
ScalarKernel flipped_kernel = *kernel;
+ const auto& in_types = kernel->signature->in_types();
+ if (in_types[0] != in_types[1]) {
+ flipped_kernel.signature = KernelSignature::Make(
+ {in_types[1], in_types[0]}, kernel->signature->out_type());
Review Comment:
This is assuming that if the input types differ, then there isn't any
constraint. This is currently true, but it's a hidden assumption and could
change in future. It would be good to either document this with a comment, or
just pass through the constraint from the original kernel even thought it's
always null at the moment.
##########
cpp/src/arrow/compute/expression_test.cc:
##########
@@ -2379,5 +2379,55 @@ TEST(Projection, AugmentWithKnownValues) {
}));
}
+TEST(Expression, TestIssue54668DecimalComparisonDifferentScales) {
Review Comment:
This also tests string comparisons, so the name is inaccurate. Can it please
be split into two tests?
--
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]
