From 8e01ed9f16984a752f7255db63132d6aa4355f43 Mon Sep 17 00:00:00 2001 From: Sasha Krassovsky Date: Fri, 5 Nov 2021 14:43:45 -0700 Subject: [PATCH] Switch to literal(true) --- cpp/src/arrow/compute/exec/expression.cc | 2 -- cpp/src/arrow/compute/exec/expression.h | 3 --- cpp/src/arrow/compute/exec/hash_join.cc | 6 +++--- cpp/src/arrow/compute/exec/hash_join.h | 8 ++++---- cpp/src/arrow/compute/exec/hash_join_node.cc | 6 +++--- cpp/src/arrow/compute/exec/hash_join_node_test.cc | 9 ++------- cpp/src/arrow/compute/exec/options.h | 6 +++--- cpp/src/arrow/compute/exec/plan_test.cc | 4 ++-- cpp/src/arrow/compute/exec/util_test.cc | 8 ++++---- 9 files changed, 21 insertions(+), 31 deletions(-) diff --git a/cpp/src/arrow/compute/exec/expression.cc b/cpp/src/arrow/compute/exec/expression.cc index 0df6345d0bbd9..64e3305825d10 100644 --- a/cpp/src/arrow/compute/exec/expression.cc +++ b/cpp/src/arrow/compute/exec/expression.cc @@ -323,8 +323,6 @@ bool Expression::IsSatisfiable() const { return true; } -bool Expression::IsEmpty() const { return impl_ == nullptr; } - namespace { // Produce a bound Expression from unbound Call and bound arguments. diff --git a/cpp/src/arrow/compute/exec/expression.h b/cpp/src/arrow/compute/exec/expression.h index ecedb5e41f495..dac5728ab46d6 100644 --- a/cpp/src/arrow/compute/exec/expression.h +++ b/cpp/src/arrow/compute/exec/expression.h @@ -91,9 +91,6 @@ class ARROW_EXPORT Expression { /// Return true if this expression could evaluate to true. bool IsSatisfiable() const; - /// Return true if this expression has no clauses. - bool IsEmpty() const; - // XXX someday // Result GetPipelines(); diff --git a/cpp/src/arrow/compute/exec/hash_join.cc b/cpp/src/arrow/compute/exec/hash_join.cc index 5ca571c66d68e..c1cf1c0513e6f 100644 --- a/cpp/src/arrow/compute/exec/hash_join.cc +++ b/cpp/src/arrow/compute/exec/hash_join.cc @@ -284,7 +284,7 @@ class HashJoinBasicImpl : public HashJoinImpl { std::vector& no_match, std::vector& match_left, std::vector& match_right) { - if (filter_.IsEmpty()) { + if (filter_ == literal(true)) { return Status::OK(); } ARROW_DCHECK_EQ(match_left.size(), match_right.size()); @@ -298,13 +298,13 @@ class HashJoinBasicImpl : public HashJoinImpl { hash_table_keys_.Decode(match_right.size(), match_right.data())); ExecBatch left_payload; - if (schema_mgr_->HasLeftPayload()) { + if (!schema_mgr_->LeftPayloadIsEmpty()) { ARROW_ASSIGN_OR_RAISE(left_payload, local_state.exec_batch_payloads.Decode( match_left.size(), match_left.data())); } ExecBatch right_payload; - if (schema_mgr_->HasRightPayload()) { + if (!schema_mgr_->RightPayloadIsEmpty()) { ARROW_ASSIGN_OR_RAISE(right_payload, hash_table_payloads_.Decode( match_right.size(), match_right.data())); } diff --git a/cpp/src/arrow/compute/exec/hash_join.h b/cpp/src/arrow/compute/exec/hash_join.h index 8df8f0c7a00c4..b62a939958140 100644 --- a/cpp/src/arrow/compute/exec/hash_join.h +++ b/cpp/src/arrow/compute/exec/hash_join.h @@ -61,9 +61,9 @@ class ARROW_EXPORT HashJoinSchema { std::shared_ptr MakeOutputSchema(const std::string& left_field_name_prefix, const std::string& right_field_name_prefix); - bool HasLeftPayload() { return HasPayload(0); } + bool LeftPayloadIsEmpty() { return PayloadIsEmpty(0); } - bool HasRightPayload() { return HasPayload(1); } + bool RightPayloadIsEmpty() { return PayloadIsEmpty(1); } static int kMissingField() { return SchemaProjectionMaps::kMissingField; @@ -78,9 +78,9 @@ class ARROW_EXPORT HashJoinSchema { Status TraverseExpression(std::vector& refs, const Expression& filter, const Schema& schema); - bool HasPayload(int side) { + bool PayloadIsEmpty(int side) { ARROW_DCHECK(side == 0 || side == 1); - return proj_maps[side].num_cols(HashJoinProjection::PAYLOAD) > 0; + return proj_maps[side].num_cols(HashJoinProjection::PAYLOAD) == 0; } static Result> ComputePayload(const Schema& schema, diff --git a/cpp/src/arrow/compute/exec/hash_join_node.cc b/cpp/src/arrow/compute/exec/hash_join_node.cc index 265d70b65fcef..71ea19fcc108a 100644 --- a/cpp/src/arrow/compute/exec/hash_join_node.cc +++ b/cpp/src/arrow/compute/exec/hash_join_node.cc @@ -309,7 +309,7 @@ Result HashJoinSchema::BindFilter(Expression filter, if (filter.IsBound()) { return std::move(filter); } - if (!filter.IsEmpty()) { + if (filter != literal(true)) { FieldVector fields; auto left = proj_maps[0].map(HashJoinProjection::FILTER, HashJoinProjection::INPUT); auto right = proj_maps[1].map(HashJoinProjection::FILTER, HashJoinProjection::INPUT); @@ -332,7 +332,7 @@ Result HashJoinSchema::BindFilter(Expression filter, } return std::move(filter); } - return Expression(); + return literal(true); } Result> HashJoinSchema::CollectFilterColumns( @@ -355,7 +355,7 @@ Result> HashJoinSchema::CollectFilterColumns( Status HashJoinSchema::TraverseExpression(std::vector& refs, const Expression& filter, const Schema& schema) { - if (filter.IsEmpty()) return Status::OK(); + if (filter == literal(true)) return Status::OK(); if (auto* call = filter.call()) { for (const Expression& arg : call->arguments) RETURN_NOT_OK(TraverseExpression(refs, arg, schema)); diff --git a/cpp/src/arrow/compute/exec/hash_join_node_test.cc b/cpp/src/arrow/compute/exec/hash_join_node_test.cc index 301c98ee16055..e801af3d02647 100644 --- a/cpp/src/arrow/compute/exec/hash_join_node_test.cc +++ b/cpp/src/arrow/compute/exec/hash_join_node_test.cc @@ -1094,7 +1094,7 @@ TEST(HashJoin, Random) { } // Turn the last key comparison into a residual filter expression - Expression filter; + Expression filter = literal(true); if (key_cmp.size() > 1) { for (size_t i = 0; i < key_cmp.size(); i++) { FieldRef left = key_fields[0][i]; @@ -1114,12 +1114,7 @@ TEST(HashJoin, Random) { } } } - - if (!filter.IsEmpty()) { - std::cout << " Filter: " << filter.ToString() << "\n"; - } else { - std::cout << " Filter: \n"; - } + std::cout << " Filter: " << filter.ToString() << "\n"; // Run tested join implementation HashJoinNodeOptions join_options{ diff --git a/cpp/src/arrow/compute/exec/options.h b/cpp/src/arrow/compute/exec/options.h index 3866dce88c5d7..89e20851b345d 100644 --- a/cpp/src/arrow/compute/exec/options.h +++ b/cpp/src/arrow/compute/exec/options.h @@ -173,7 +173,7 @@ class ARROW_EXPORT HashJoinNodeOptions : public ExecNodeOptions { static constexpr const char* default_output_prefix_for_right = ""; HashJoinNodeOptions( JoinType in_join_type, std::vector in_left_keys, - std::vector in_right_keys, Expression filter = Expression(), + std::vector in_right_keys, Expression filter = literal(true), std::string output_prefix_for_left = default_output_prefix_for_left, std::string output_prefix_for_right = default_output_prefix_for_right) : join_type(in_join_type), @@ -191,7 +191,7 @@ class ARROW_EXPORT HashJoinNodeOptions : public ExecNodeOptions { HashJoinNodeOptions( JoinType join_type, std::vector left_keys, std::vector right_keys, std::vector left_output, - std::vector right_output, Expression filter = Expression(), + std::vector right_output, Expression filter = literal(true), std::string output_prefix_for_left = default_output_prefix_for_left, std::string output_prefix_for_right = default_output_prefix_for_right) : join_type(join_type), @@ -212,7 +212,7 @@ class ARROW_EXPORT HashJoinNodeOptions : public ExecNodeOptions { JoinType join_type, std::vector left_keys, std::vector right_keys, std::vector left_output, std::vector right_output, std::vector key_cmp, - Expression filter = Expression(), + Expression filter = literal(true), std::string output_prefix_for_left = default_output_prefix_for_left, std::string output_prefix_for_right = default_output_prefix_for_right) : join_type(join_type), diff --git a/cpp/src/arrow/compute/exec/plan_test.cc b/cpp/src/arrow/compute/exec/plan_test.cc index 7a25f04ded095..267fcf2de4b5f 100644 --- a/cpp/src/arrow/compute/exec/plan_test.cc +++ b/cpp/src/arrow/compute/exec/plan_test.cc @@ -1042,7 +1042,7 @@ TEST(ExecPlanExecution, SelfInnerHashJoinSink) { HashJoinNodeOptions join_opts{JoinType::INNER, /*left_keys=*/{"str"}, - /*right_keys=*/{"str"}, Expression(), "l_", "r_"}; + /*right_keys=*/{"str"}, literal(true), "l_", "r_"}; ASSERT_OK_AND_ASSIGN( auto hashjoin, @@ -1099,7 +1099,7 @@ TEST(ExecPlanExecution, SelfOuterHashJoinSink) { HashJoinNodeOptions join_opts{JoinType::FULL_OUTER, /*left_keys=*/{"str"}, - /*right_keys=*/{"str"}, Expression(), "l_", "r_"}; + /*right_keys=*/{"str"}, literal(true), "l_", "r_"}; ASSERT_OK_AND_ASSIGN( auto hashjoin, diff --git a/cpp/src/arrow/compute/exec/util_test.cc b/cpp/src/arrow/compute/exec/util_test.cc index 82b386e5c1c53..6f4b5315fff5b 100644 --- a/cpp/src/arrow/compute/exec/util_test.cc +++ b/cpp/src/arrow/compute/exec/util_test.cc @@ -35,7 +35,7 @@ TEST(FieldMap, Trivial) { auto right = schema({field("i32", int32())}); ASSERT_OK(schema_mgr.Init(JoinType::INNER, *left, {"i32"}, *right, {"i32"}, - Expression(), kLeftPrefix, kRightPrefix)); + literal(true), kLeftPrefix, kRightPrefix)); auto output = schema_mgr.MakeOutputSchema(kLeftPrefix, kRightPrefix); EXPECT_THAT(*output, Eq(Schema({ @@ -55,7 +55,7 @@ TEST(FieldMap, TrivialDuplicates) { auto right = schema({field("i32", int32())}); ASSERT_OK(schema_mgr.Init(JoinType::INNER, *left, {"i32"}, *right, {"i32"}, - Expression(), "", "")); + literal(true), "", "")); auto output = schema_mgr.MakeOutputSchema("", ""); EXPECT_THAT(*output, Eq(Schema({ @@ -75,7 +75,7 @@ TEST(FieldMap, SingleKeyField) { auto right = schema({field("f32", float32()), field("i32", int32())}); ASSERT_OK(schema_mgr.Init(JoinType::INNER, *left, {"i32"}, *right, {"i32"}, - Expression(), kLeftPrefix, kRightPrefix)); + literal(true), kLeftPrefix, kRightPrefix)); EXPECT_EQ(schema_mgr.proj_maps[0].num_cols(HashJoinProjection::INPUT), 2); EXPECT_EQ(schema_mgr.proj_maps[1].num_cols(HashJoinProjection::INPUT), 2); @@ -113,7 +113,7 @@ TEST(FieldMap, TwoKeyFields) { }); ASSERT_OK(schema_mgr.Init(JoinType::INNER, *left, {"i32", "str"}, *right, - {"i32", "str"}, Expression(), kLeftPrefix, kRightPrefix)); + {"i32", "str"}, literal(true), kLeftPrefix, kRightPrefix)); auto output = schema_mgr.MakeOutputSchema(kLeftPrefix, kRightPrefix); EXPECT_THAT(*output, Eq(Schema({