nealrichardson commented on a change in pull request #9294:
URL: https://github.com/apache/arrow/pull/9294#discussion_r570550172
##########
File path: cpp/src/arrow/dataset/expression_test.cc
##########
@@ -57,40 +57,91 @@ void ExpectResultsEqual(Actual&& actual, Expected&&
expected) {
MaybeExpected maybe_expected(std::forward<Expected>(expected));
if (maybe_expected.ok()) {
- ASSERT_OK_AND_ASSIGN(auto actual, maybe_actual);
- EXPECT_EQ(actual, *maybe_expected);
+ EXPECT_EQ(maybe_actual, maybe_expected);
} else {
- EXPECT_EQ(maybe_actual.status().code(), expected.status().code());
-
EXPECT_NE(maybe_actual.status().message().find(expected.status().message()),
- std::string::npos)
- << " actual: " << maybe_actual.status() << "\n"
- << " expected: " << maybe_expected.status();
+ EXPECT_RAISES_WITH_CODE_AND_MESSAGE_THAT(
+ expected.status().code(), HasSubstr(expected.status().message()),
maybe_actual);
}
}
+const auto no_change = util::nullopt;
+
TEST(ExpressionUtils, Comparison) {
auto Expect = [](Result<std::string> expected, Datum l, Datum r) {
ExpectResultsEqual(Comparison::Execute(l, r).Map(Comparison::GetName),
expected);
};
- Datum zero(0), one(1), two(2), null(std::make_shared<Int32Scalar>()),
str("hello");
+ Datum zero(0), one(1), two(2), null(std::make_shared<Int32Scalar>());
+ Datum str("hello"),
bin(std::make_shared<BinaryScalar>(Buffer::FromString("hello")));
+ Datum dict_str(DictionaryScalar::Make(std::make_shared<Int32Scalar>(0),
+ ArrayFromJSON(utf8(), R"(["a", "b",
"c"])")));
- Status parse_failure = Status::Invalid("Failed to parse");
+ Status not_impl = Status::NotImplemented("no kernel matching input types");
Expect("equal", one, one);
Expect("less", one, two);
Expect("greater", one, zero);
- // cast RHS to LHS type; "hello" > "1"
- Expect("greater", str, one);
- // cast RHS to LHS type; "hello" is not convertible to int
- Expect(parse_failure, one, str);
-
Expect("na", one, null);
- Expect("na", str, null);
Expect("na", null, one);
- // cast RHS to LHS type; "hello" is not convertible to int
- Expect(parse_failure, null, str);
+
+ // strings and ints are not comparable without explicit casts
+ Expect(not_impl, str, one);
+ Expect(not_impl, one, str);
+ Expect(not_impl, str, null); // not even null ints
+
+ // string -> binary implicit cast allowed
+ Expect("equal", str, bin);
+ Expect("equal", bin, str);
+
+ // dict_str -> string, implicit casts allowed
+ Expect("less", dict_str, str);
+ Expect("less", dict_str, bin);
+}
+
+TEST(ExpressionUtils, StripOrderPreservingCasts) {
+ auto Expect = [](Expression expr, util::optional<Expression>
expected_stripped) {
+ ASSERT_OK_AND_ASSIGN(expr, expr.Bind(*kBoringSchema));
+ if (!expected_stripped) {
+ expected_stripped = expr;
+ } else {
+ ASSERT_OK_AND_ASSIGN(expected_stripped,
expected_stripped->Bind(*kBoringSchema));
+ }
+ EXPECT_EQ(Comparison::StripOrderPreservingCasts(expr), *expected_stripped);
+ };
+
+ // Casting int to float preserves ordering.
+ // For example, let
+ // a = 3, b = 2, assert(a > b)
+ // After injecting a cast to float, this ordering still holds
+ // float(a) == 3.0, float(b) == 2.0, assert(float(a) > float(b))
Review comment:
Maybe the comment isn't strictly valid for all possible values. This
seems like a rather extreme edge case--I suggest we ticket this for followup.
On balance this PR add lots of value and fixes many other (much more common)
issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
