bkietz commented on a change in pull request #9294:
URL: https://github.com/apache/arrow/pull/9294#discussion_r572398811
##########
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:
I've created https://issues.apache.org/jira/browse/ARROW-11562 to make
this logic more robust
----------------------------------------------------------------
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
