feat: implement Primitive type Literal #117

mapleFU · 2025-06-04T15:12:06Z

Before implement expression, Primitive should be implemented. The spec follows: https://iceberg.apache.org/spec/#schemas-and-data-types

In this patch, only few types of Literals is supported, including int, long, float, double, boolean, string and binary.

TODO:

Implement detail logics about comparing
Implement detail logics about castTo
ToString and printing logics
Tests

Would in other patch:

More primitive type basic supports ( maybe without decimal now )
Implement detail logics about serialize/deserialize

mapleFU · 2025-06-04T15:48:35Z

src/iceberg/datum.h

+                 int64_t,               // for long, timestamp, timestamp_tz, time
+                 float,                 // for float
+                 double,                // for double
+                 std::string,           // for string


C++ string not requires utf8, so I wonder whether std::vector<uint8_t> with string is possible

Small String Optimization (SSO) will make PrimitiveLiteralValue 40 bytes, may be use a unique_ptr to own the string?

Just a nit, Literal might be used as vector, so more bytes is not critical?

I think the data part of vector will not be in Literal's layout?
If we are not going to use something like std::vector, I think it's fine with the current design.

Emmm, a tiny wrapper like this should be defined if unique_ptr should be added. I'm also ok for the case

struct StringLiteralValue { StringLiteralValue(std::string value); StringLiteralValue(const StringLiteralValue&); StringLiteralValue(StringLiteralValue&&) noexcept; const std::string& str() const { return *value_; } bool operator==(const StringLiteralValue&) const; std::strong_ordering operator<=>(const StringLiteralValue&) const; private: std::unique_ptr<std::string> value_; };

In libc++ version, sizeof(PrimitiveLiteralValue) == 48, since std::vector is large, it's still 48b after change to StringLiteralValue.

After change to below:

template <typename T> struct LiteralValueWrapper { LiteralValueWrapper(T value); LiteralValueWrapper(const LiteralValueWrapper&); LiteralValueWrapper(LiteralValueWrapper&&) noexcept = default; const std::string& str() const { return *value_; } bool operator==(const LiteralValueWrapper<T>&) const; std::strong_ordering operator<=>(const LiteralValueWrapper<T>&) const; private: std::unique_ptr<T> value_; }; using PrimitiveLiteralValue = std::variant<bool, // for boolean int32_t, // for int, date int64_t, // for long, timestamp, timestamp_tz, time float, // for float double, // for double LiteralValueWrapper<std::string>, // for string LiteralValueWrapper<std::vector<uint8_t>>, // for binary, fixed std::array<uint8_t, 16>, // for uuid and decimal BelowMin, AboveMax>;

The sizeof sizeof(PrimitiveLiteralValue) == 48 becomes 40B, just eliminate 8B 😅. I think it's due to C++ doesn't do any niche optimization for std::variant... If the largest element in PrimitiveLiteralValue is 8B, sizeof(PrimitiveLiteralValue) can be 32B. So I think currently std::string is enough here?

Thanks for the analysis, let's keep std::string :)

src/iceberg/datum.h

mapleFU · 2025-06-04T15:49:10Z

src/iceberg/datum.h

+
+ private:
+  PrimitiveLiteralValue value_;
+  std::shared_ptr<PrimitiveType> type_;


don't know std::shared_ptr<PrimitiveType> to heavy for this type

wgtmac · 2025-06-05T06:57:24Z

As discussed with @mapleFU offline, we need a good abstraction on Literal to represent arbitrary nesting structure (e.g. primitive, struct, map, list, and even geometry and variant?) and optimize specifically for primitive types.

As far as I know, literal values will be used in the following cases:

Literal value in a predicate.
Partition value in the DataFile.
Default value in a SchemaField for Iceberg V3.
Input and output values for a transform function.

For expression evaluator, we can directly evaluate the Arrow arrays as all file readers support reading into Arrow arrays so we don't pay for any data conversion. We can also specialize expression evaluators for ManifestFile, Snapshot and ContentFile. We can also provide a visitor to visit Arrow arrays to get the literal values of a single row with selected columns to make it easy to use.

If literals are all we need, we don't have to implement a StructLike in #77.

WDYT? @dongxiao1198 @gty404 @lidavidm @zhjwpku (names are sorted alphabetically :）)

zhjwpku · 2025-06-05T11:24:10Z

src/iceberg/datum.h

+                 BelowMin, AboveMax>;
+
+/// \brief PrimitiveLiteral is owned literal of a primitive type.
+class PrimitiveLiteral {


Rename to Datum since you are using datum.h/cc as the filename?
Personally I prefer the following:

using PrimitiveLiteral = std::variant<bool, ... class Datum { PrimitiveLiteral literal_, std::shared_ptr<PrimitiveType> };

src/iceberg/datum.h

zhjwpku · 2025-06-05T11:55:20Z

src/iceberg/datum.h

+                 int64_t,               // for long, timestamp, timestamp_tz, time
+                 float,                 // for float
+                 double,                // for double
+                 std::string,           // for string


Small String Optimization (SSO) will make PrimitiveLiteralValue 40 bytes, may be use a unique_ptr to own the string?

lidavidm · 2025-06-05T12:58:23Z

The idea seems reasonable to me.

src/iceberg/literal.cc

mapleFU · 2025-06-05T15:43:45Z

Would start testing if interface is ok

src/iceberg/literal.h

zhjwpku

LGTM

yingcai-cy · 2025-06-09T09:58:59Z

src/iceberg/literal.h

+  static PrimitiveLiteral Float(float value);
+  static PrimitiveLiteral Double(double value);
+  static PrimitiveLiteral String(std::string value);
+  static PrimitiveLiteral Binary(std::vector<uint8_t> value);


Can UUIDLiteral and DecimalLiteral also be created via factory methods?

Yes, we can delay it into future patches.

I think we need. Otherwise we don't know the exact type of the literal.

yingcai-cy · 2025-06-09T10:14:08Z

src/iceberg/literal.h

+ public:
+  /// Factory methods for primitive types
+  static PrimitiveLiteral Boolean(bool value);
+  static PrimitiveLiteral Int(int32_t value);


Not sure if we should treat Timestamp and Date differently from long/int in expressions or elsewhere.

Sure, we can uses more factory here. This patch only supports int, float, string, binary. We can first make this in so we can then write the (1) impl logic for other literals (2) basic implement for predicate expressions.

yingcai-cy · 2025-06-09T10:19:12Z

src/iceberg/literal.cc

+      break;
+  }
+
+  return NotSupported("Cast from {} to {} is not implemented", type_->ToString(),


not implemented -> not supported?

yingcai-cy

LGTM

wgtmac · 2025-06-06T01:11:19Z

src/iceberg/CMakeLists.txt

@@ -24,6 +24,7 @@ set(ICEBERG_SOURCES
    expression/expression.cc
    file_reader.cc
    json_internal.cc
+    literal.cc


Should it be put into the expression subdir?

wgtmac · 2025-06-06T01:13:38Z

src/iceberg/literal.h

+                   int64_t,                  // for long, timestamp, timestamp_tz, time
+                   float,                    // for float
+                   double,                   // for double
+                   std::string,              // for string


Do we need a string_view variant?

PrimitiveLiteral is "owned", I think using string_view is high risk and most of time doesn't bring up performance

wgtmac · 2025-06-20T06:04:46Z

src/iceberg/literal.h

+namespace iceberg {
+
+/// \brief PrimitiveLiteral is a literal value that is associated with a primitive type.
+class ICEBERG_EXPORT PrimitiveLiteral {


Should we simply use Literal instead of PrimitiveLiteral which looks lengthy?

I afraid in the future there would be MapLiteral and StructLiteral. But all is ok to me

MapLiteral and StructLiteral may be combination of Literals and the majority use case is still for primitive types. So I believe making them shorter looks better. @Fokko WDYT?

src/iceberg/literal.h

wgtmac · 2025-06-20T06:19:41Z

src/iceberg/literal.h

+                   float,                    // for float
+                   double,                   // for double
+                   std::string,              // for string
+                   std::vector<uint8_t>,     // for binary, fixed


Why not reusing std::string?

It's a good idea. C++ doesn't have a concept for "utf8 encoded string", here I would like to use std::string to repr this. But std::vector<uint8_t> or std::string all looks ok to me

wgtmac · 2025-06-20T06:22:37Z

src/iceberg/literal.h

+  static PrimitiveLiteral Float(float value);
+  static PrimitiveLiteral Double(double value);
+  static PrimitiveLiteral String(std::string value);
+  static PrimitiveLiteral Binary(std::vector<uint8_t> value);


I think we need. Otherwise we don't know the exact type of the literal.

src/iceberg/literal.h

wgtmac · 2025-06-20T06:27:45Z

src/iceberg/literal.h

+ private:
+  PrimitiveLiteral(PrimitiveLiteralValue value, std::shared_ptr<PrimitiveType> type);
+
+  static PrimitiveLiteral BelowMinLiteral(std::shared_ptr<PrimitiveType> type);


I don't think we need a type here, right? If your intention is to make them comparable, I think we can add ToAboveMax() and ToBelowMin() to replace these two static functions to keep the original type.

I don't think we need a type here, right?

BelowMin and AboveMax should be typed, a BelowMin for int32 and other types have different meaning

sample: Literal(type:int64, value: int32_t::max() + 1) cast to int32_t becomes Literal(type:int32, value: AboveMax)

…pp into primitive-literal-impl

src/iceberg/expression/literal.h

wgtmac · 2025-06-20T13:47:16Z

src/iceberg/expression/literal.h

+    std::strong_ordering operator<=>(const AboveMax&) const = default;
+  };
+
+  using Value = std::variant<bool,         // for boolean


Should we make it public so users are able to get the value?

We can expose this in the future, but currently I think encapsulate in here is ok

src/iceberg/expression/literal.h

wgtmac · 2025-06-20T14:06:06Z

src/iceberg/expression/literal.cc

+    case TypeId::kTime:
+    case TypeId::kTimestamp:
+    case TypeId::kTimestampTz: {
+      throw IcebergError("Not implemented: ToString for " + type_->ToString());


Please do not throw

Emmm what should we do here? Here I want to first mark it as unimplemented, when all literal is implemented, this should be change to std::string. Should this first as:

return fmt::format("Not implemented: ToString for {}", type_->ToString());

Or

Result<std::string> .ToString()

?

src/iceberg/expression/literal.cc

Co-authored-by: Gang Wu <ustcwg@gmail.com>

mapleFU added 3 commits June 4, 2025 23:10

init templates

59f60da

some minor enhancement

9355670

try to fix lint

082c7e1

mapleFU force-pushed the primitive-literal-impl branch from 8285c07 to 082c7e1 Compare June 4, 2025 15:47

mapleFU commented Jun 4, 2025

View reviewed changes

zhjwpku reviewed Jun 5, 2025

View reviewed changes

mapleFU added 3 commits June 5, 2025 22:56

Resolve comments and add impl for compare / cast

e1dd11d

Fix some logic for using datum

1132ef5

Rename files

1eb9a9b

mapleFU commented Jun 5, 2025

View reviewed changes

src/iceberg/literal.cc Outdated Show resolved Hide resolved

mapleFU marked this pull request as ready for review June 5, 2025 15:42

zhjwpku reviewed Jun 6, 2025

View reviewed changes

Resolve some comments, and remove castFromDouble

07dd257

zhjwpku approved these changes Jun 6, 2025

View reviewed changes

Add basic tests, and fix float compare bug(qNan != sNan)

146a86e

mapleFU force-pushed the primitive-literal-impl branch from d6b7e8e to 146a86e Compare June 6, 2025 13:33

Fix lint

d125408

yingcai-cy reviewed Jun 9, 2025

View reviewed changes

some NotImplement -> NotSupported

012966f

yingcai-cy approved these changes Jun 10, 2025

View reviewed changes

wgtmac suggested changes Jun 20, 2025

View reviewed changes

mapleFU added 4 commits June 20, 2025 16:03

Move literal to expression dir

88f67dc

Apply suggestions and create PrimitiveLiteralCaster

694cb55

Merge branch 'primitive-literal-impl' of github.com:mapleFU/iceberg-c…

6d72f80

…pp into primitive-literal-impl

rename variables

1d7f904

mapleFU changed the title ~~feat: implement PrimitiveLiteral~~ feat: implement Primitive type Literal Jun 20, 2025

wgtmac suggested changes Jun 20, 2025

View reviewed changes

mapleFU and others added 4 commits June 20, 2025 22:49

Apply suggestions from code review

470259d

Co-authored-by: Gang Wu <ustcwg@gmail.com>

Resolve comments

56c265e

continue resolve comments

c46ff60

Fix lint

197102e

feat: implement Primitive type Literal #117

Are you sure you want to change the base?

feat: implement Primitive type Literal #117

Uh oh!

Conversation

mapleFU commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Jun 5, 2025

Uh oh!

Uh oh!

mapleFU commented Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhjwpku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yingcai-cy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jun 4, 2025 •

edited

Loading

mapleFU Jun 6, 2025 •

edited

Loading