Switch to literal(true)

apache · Nov 6, 2021 · 8e01ed9 · 8e01ed9
1 parent 9f52ab7
commit 8e01ed9
Show file tree

Hide file tree

Showing 9 changed files with 21 additions and 31 deletions.
diff --git 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
@@ -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<PipelineGraph> GetPipelines();
 

diff --git 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<int32_t>& no_match,
                                    std::vector<int32_t>& match_left,
                                    std::vector<int32_t>& 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
@@ -61,9 +61,9 @@ class ARROW_EXPORT HashJoinSchema {
   std::shared_ptr<Schema> 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<HashJoinProjection>::kMissingField;
@@ -78,9 +78,9 @@ class ARROW_EXPORT HashJoinSchema {
   Status TraverseExpression(std::vector<FieldRef>& 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<std::vector<FieldRef>> 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
@@ -309,7 +309,7 @@ Result<Expression> 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<Expression> HashJoinSchema::BindFilter(Expression filter,
     }
     return std::move(filter);
   }
-  return Expression();
+  return literal(true);
 }
 
 Result<std::vector<FieldRef>> HashJoinSchema::CollectFilterColumns(
@@ -355,7 +355,7 @@ Result<std::vector<FieldRef>> HashJoinSchema::CollectFilterColumns(
 Status HashJoinSchema::TraverseExpression(std::vector<FieldRef>& 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
@@ -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: <empty>\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
@@ -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<FieldRef> in_left_keys,
-      std::vector<FieldRef> in_right_keys, Expression filter = Expression(),
+      std::vector<FieldRef> 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<FieldRef> left_keys,
       std::vector<FieldRef> right_keys, std::vector<FieldRef> left_output,
-      std::vector<FieldRef> right_output, Expression filter = Expression(),
+      std::vector<FieldRef> 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<FieldRef> left_keys,
       std::vector<FieldRef> right_keys, std::vector<FieldRef> left_output,
       std::vector<FieldRef> right_output, std::vector<JoinKeyCmp> 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
@@ -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
@@ -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({