feat: introduce Scalar and StructLike interfaces for Transform evaluation #77

gty404 · 2025-04-16T09:35:52Z

Problems to Solve

nanoarrow does not provide a copy interface for ArrowArray, which is needed when implementing IdentityTransform.
Both Transform and Expression would benefit from a lightweight, uniform data structure for input values — ideally something like a simplified StructLike.

Proposed Solution

Introduce an abstract Scalar interface to represent values (both primitive and complex).
Define a StructLike interface to represent structured values with field-based access.
Allow classes such as ManifestEntity, DataFile, etc., to implement StructLike and serve as evaluation inputs for Transform and Expression.

Benefits

Decouples Transform/Expression from ArrowArray, improving flexibility.
Enables easier testing, mocking, and transformation logic using in-memory representations.
Lays the foundation for future enhancements like expression optimization or code generation.

…tion

wgtmac · 2025-04-16T15:42:47Z

src/iceberg/struct_like.h

+  virtual ~ScalarVisitor() = default;
+
+  /// Visit methods for primitive types
+  virtual void visit(int32_t value) = 0;


Shouldn't it accept Int32Scalar?

The PrimitiveScalar::Accept method directly uses value, and using PrimitiveScalar is also acceptable. I will make some modifications.

If it accepts physical values instead of typed values, how can it differentiate between the actual types? For example, long and timestamp both use int64_t.

wgtmac · 2025-04-16T15:44:17Z

src/iceberg/struct_like.h

+  std::optional<T> value_;
+};
+
+// Aliases for common primitive scalar types


We are supposed to specialize each type like DecimalType, BinaryType, etc.?

I think it should be specialized.

wgtmac · 2025-04-16T15:47:04Z

src/iceberg/struct_like.h

+/**
+ * @brief Base class representing a generic scalar value.
+ */
+class ICEBERG_EXPORT Scalar {


Should we make std::shared_ptr<Type> as a member variable to Scalar?

There are three possible approaches to organizing type information:

Scalar holds built-in types independently, and StructLike does not rely on StructType.

Scalar does not contain any type information, and its type is inferred from the associated StructType within StructLike.

Both Scalar and StructType carry type information, resulting in some redundancy.
I have chosen the first approach. Which one would you prefer?

If Scalar does not carry the type info, we have to pass Schema wherever Scalar is used. In the case of schema evolution, then we need both Scalar schema and expected (table) schema.

wgtmac · 2025-04-16T15:54:34Z

src/iceberg/struct_like.h

+ * @tparam Derived The concrete subclass type (CRTP).
+ */
+template <typename Derived>
+class VectorScalarBase : public Scalar {


It is a little bit weird that values in it have different meanings in the StructScalar (horizontal) and ArrayScalar (vertical).

It is a bit strange because the type information is all in StructLike, with StructScalar/ArrayScalar both being a group of Scalar members and there is not much difference.

But this sounds like an anti-pattern in the OOP. You cannot use the base VectorScalarBase class without knowing its subclass implementation.

wgtmac · 2025-04-16T15:55:25Z

src/iceberg/struct_like.h

+/**
+ * @brief Represents an array-like scalar (list of values).
+ */
+class ArrayScalar : public VectorScalarBase<ArrayScalar> {


How do we define MapScalar? Should it be array<struct<key,value>>?

Currently, I haven't thought of a good design, perhaps map can still be used, and Scalar needs to add interfaces for hash and equality judgment?

wgtmac · 2025-04-16T15:58:21Z

src/iceberg/struct_like.h

+/**
+ * @brief Abstract interface for structured field access.
+ */
+class ICEBERG_EXPORT StructLike {


Can't we simply reuse StructScalar?

StructLike has a StructType, while StructScalar does not have one, I think this should depend on the discussion result regarding whether a Scalar should have its own type information.

wgtmac · 2025-04-16T15:58:40Z

src/iceberg/struct_like.h

+/**
+ * @brief Visitor interface for traversing StructLike fields.
+ */
+class ICEBERG_EXPORT StructLikeVisitor {


ditto, we can remove this as well.

lidavidm · 2025-04-17T00:22:49Z

src/iceberg/struct_like.h

+   * @brief Checks if the scalar represents a null value.
+   * @return true if the value is null, false otherwise.
+   */
+  virtual bool IsNull() const = 0;


Is the implication that this is not a non-trivial operation? (Else it might be is_null to imply it's a simple getter?)

In the Scalar class, is_null can be a member variable, or the IsNull interface can be implemented by subclasses, where subclasses might use optional or pointer to determine if it is null. Here, the second option was chosen; I actually don't have a strong preference. StructScalar also uses the is_null field, so it might be more appropriate to directly make is_null a member variable of Scalar. I'll make a modification.

lidavidm · 2025-04-17T00:23:01Z

src/iceberg/struct_like.h

+   * @brief Creates a deep copy of the scalar.
+   * @return A shared pointer to the cloned scalar.
+   */
+  virtual std::shared_ptr<Scalar> clone() const = 0;


Shouldn't this be Clone? (Ditto in general for the PR)

Thanks, I will modify it.

lidavidm · 2025-04-17T00:23:58Z

src/iceberg/struct_like.h

+  /**
+   * @brief Called when visiting a null scalar.
+   */
+  virtual void visitNull() = 0;


Is this a specific NullScalar type, or any scalar with a null value?

Ok I see the behavior below. IMO I'd rather we pass the scalars to the visitor so that the users can tell between different types of nulls

Good suggestion, visit I will modify it to accept Scalar types as input

lidavidm · 2025-04-17T00:25:08Z

src/iceberg/struct_like.h

+   * @return Reference to the value.
+   * @note Behavior is undefined if the scalar is null.
+   */
+  const T& value() const { return *value_; }


nit but perhaps in DEBUG it should use optional::value() to force an exception and in NDEBUG it can use operator* to bypass that

Good suggestion, I will modify here

feat: introduce Scalar and StructLike interfaces for Transform evalua…

e73f2cc

…tion

wgtmac reviewed Apr 16, 2025

View reviewed changes

lidavidm reviewed Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce Scalar and StructLike interfaces for Transform evaluation #77

feat: introduce Scalar and StructLike interfaces for Transform evaluation #77

gty404 commented Apr 16, 2025

wgtmac Apr 16, 2025

gty404 Apr 17, 2025

wgtmac Apr 18, 2025 •

edited

Loading

wgtmac Apr 16, 2025

gty404 Apr 17, 2025

wgtmac Apr 16, 2025

gty404 Apr 17, 2025

wgtmac Apr 18, 2025

wgtmac Apr 16, 2025

gty404 Apr 17, 2025

wgtmac Apr 18, 2025

wgtmac Apr 16, 2025

gty404 Apr 17, 2025

wgtmac Apr 16, 2025

gty404 Apr 17, 2025

wgtmac Apr 16, 2025

lidavidm Apr 17, 2025

gty404 Apr 17, 2025

lidavidm Apr 17, 2025

gty404 Apr 17, 2025

lidavidm Apr 17, 2025

lidavidm Apr 17, 2025

gty404 Apr 17, 2025

lidavidm Apr 17, 2025

gty404 Apr 17, 2025

feat: introduce Scalar and StructLike interfaces for Transform evaluation #77

Are you sure you want to change the base?

feat: introduce Scalar and StructLike interfaces for Transform evaluation #77

Conversation

gty404 commented Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac Apr 18, 2025 •

edited

Loading