feat: expression interface apply review suggestions.

Author: yingcai-cy

src/iceberg/CMakeLists.txt

@@ -20,7 +20,6 @@ set(ICEBERG_INCLUDES "$<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/src>"
 set(ICEBERG_SOURCES
     arrow_c_data_internal.cc
     demo.cc
-    expression.cc
     json_internal.cc
     schema.cc
     schema_field.cc
@@ -32,7 +31,10 @@ set(ICEBERG_SOURCES
     statistics_file.cc
     table_metadata.cc
     transform.cc
-    type.cc)
+    type.cc
+    expressions/true.cc
+    expressions/false.cc
+    expressions/and.cc)
 
 set(ICEBERG_STATIC_BUILD_INTERFACE_LIBS)
 set(ICEBERG_SHARED_BUILD_INTERFACE_LIBS)

src/iceberg/expression.cc

@@ -20,13 +20,11 @@
 #pragma once
 
 /// \file iceberg/expression.h
-/// Boolean expression tree for Iceberg table operations.
+/// Expression interface for Iceberg table operations.
 
-#include <string>
-
-#include "iceberg/error.h"
 #include "iceberg/expected.h"
 #include "iceberg/iceberg_export.h"
+#include "iceberg/result.h"
 
 namespace iceberg {
 
@@ -63,39 +61,23 @@ class ICEBERG_EXPORT Expression {
   virtual ~Expression() = default;
 
   /// \brief Returns the operation for an expression node.
-  [[nodiscard]] virtual Operation Op() const = 0;
+  virtual Operation Op() const = 0;
 
   /// \brief Returns the negation of this expression, equivalent to not(this).
-  [[nodiscard]] virtual expected<std::shared_ptr<Expression>, Error> Negate() const {
-    return unexpected<Error>(
-        {ErrorKind::kInvalidExpression, "This expression cannot be negated"});
+  virtual Result<std::shared_ptr<Expression>> Negate() const {
+    return unexpected(
+        Error(ErrorKind::kInvalidExpression, "Expression cannot be negated"));
   }
 
   /// \brief Returns whether this expression will accept the same values as another.
-  ///
-  /// If this returns true, the expressions are guaranteed to return the same evaluation
-  /// for the same input. However, if this returns false the expressions may return the
-  /// same evaluation for the same input. That is, expressions may be equivalent even if
-  /// this returns false.
-  ///
-  /// For best results, rewrite not and bind expressions before calling this method.
-  ///
   /// \param other another expression
   /// \return true if the expressions are equivalent
-  [[nodiscard]] virtual bool IsEquivalentTo(const Expression& other) const {
+  virtual bool IsEquivalentTo(const Expression& other) const {
     // only bound predicates can be equivalent
     return false;
   }
 
-  /// \brief Convert operation string to enum
-  static expected<Operation, Error> FromString(const std::string& operation_type);
-
-  /// \brief Returns the operation used when this is negated.
-  static expected<Operation, Error> Negate(Operation op);
-
-  /// \brief Returns the equivalent operation when the left and right operands are
-  /// exchanged.
-  static expected<Operation, Error> FlipLR(Operation op);
+  virtual std::string ToString() const { return "Expression"; }
 };
 
 }  // namespace iceberg

src/iceberg/expression.h

@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "and.h"
+
+namespace iceberg {
+
+And::And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right)
+    : left_(std::move(left)), right_(std::move(right)) {}
+
+bool And::IsEquivalentTo(const Expression& expr) const {
+  if (expr.Op() == Operation::kAnd) {
+    const auto& other = static_cast<const And&>(expr);
+    return (left_->IsEquivalentTo(*other.left()) &&
+            right_->IsEquivalentTo(*other.right())) ||
+           (left_->IsEquivalentTo(*other.right()) &&
+            right_->IsEquivalentTo(*other.left()));
+  }
+  return false;
+}
+
+}  // namespace iceberg

src/iceberg/expressions/and.cc

@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <format>
+#include <memory>
+#include <string>
+
+#include "iceberg/expression.h"
+
+namespace iceberg {
+
+/// \brief An Expression that represents a logical AND operation between two expressions.
+///
+/// This expression evaluates to true if and only if both of its child expressions
+/// evaluate to true.
+class ICEBERG_EXPORT And : public Expression {
+ public:
+  /// \brief Constructs an And expression from two sub-expressions.
+  ///
+  /// @param left The left operand of the AND expression
+  /// @param right The right operand of the AND expression
+  And(std::shared_ptr<Expression> left, std::shared_ptr<Expression> right);
+
+  /// \brief Returns the left operand of the AND expression.
+  ///
+  /// @return The left operand of the AND expression
+  const std::shared_ptr<Expression>& left() const { return left_; }
+
+  /// \brief Returns the right operand of the AND expression.
+  ///
+  /// @return The right operand of the AND expression
+  const std::shared_ptr<Expression>& right() const { return right_; }
+
+  Operation Op() const override { return Operation::kAnd; }
+
+  std::string ToString() const override {
+    return std::format("({} and {})", left_->ToString(), right_->ToString());
+  }
+
+  // implement Negate later
+  // expected<std::shared_ptr<Expression>, Error> Negate() const override;
+
+  bool IsEquivalentTo(const Expression& other) const override;
+
+ private:
+  std::shared_ptr<Expression> left_;
+  std::shared_ptr<Expression> right_;
+};
+
+}  // namespace iceberg

src/iceberg/expressions/and.h

@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "false.h"
+
+#include "true.h"
+
+namespace iceberg {
+
+expected<std::shared_ptr<Expression>, Error> False::Negate() const {
+  return True::shared_instance();
+}
+
+}  // namespace iceberg