feat: add transform project #371

zhjwpku · 2025-11-29T10:25:47Z

Add Transform::Project for inclusive predicate projection

This PR implements Transform::Project, which transforms a BoundPredicate to an inclusive predicate on partition values. StrictProject will be added in a separate PR to keep the review easier.

Move template implementations of Expressions into header file, or the linker will shout out the following:

Undefined symbols for architecture x86_64:
"std::__1::shared_ptr<iceberg::UnboundPredicateImpliceberg::BoundReference> iceberg::Expressions::Iniceberg::BoundReference(std::__1::shared_ptr<iceberg::UnboundTermiceberg::BoundReference>, std::initializer_listiceberg::Literal)", referenced from:
iceberg::ProjectionUtil::FixInclusiveTimeProjection(std::__1::shared_ptr<iceberg::UnboundPredicateImpliceberg::BoundReference> const&) in transform.cc.o

src/iceberg/util/projection_util_internal.h

wgtmac

I haven't finished review yet, just post some comments so far.

src/iceberg/type_fwd.h

wgtmac · 2025-12-01T07:34:27Z

src/iceberg/transform.h

+  /// \param predicate The predicate to project.
+  /// \return A Result containing either a shared pointer to the projected predicate or an
+  /// Error if the projection fails.
+  Result<std::shared_ptr<UnboundPredicate>> Project(


Can we return std::unique_ptr instead?

wgtmac · 2025-12-01T07:41:27Z

src/iceberg/util/projection_util_internal.h

+      ICEBERG_ASSIGN_OR_RAISE(auto transformed_lit, func->Transform(lit));
+      transformed.push_back(std::move(transformed_lit));
+    }
+    return Expressions::Predicate(predicate->op(), std::string(name),


We cannot use convenience functions in the Expressions because they may throw and they are only supposed to be used by users. We need to stick with Make functions to deal with any error.

src/iceberg/util/projection_util_internal.h

wgtmac · 2025-12-01T10:00:33Z

src/iceberg/util/projection_util_internal.h

+                                  std::move(transformed));
+  }
+
+  static Result<std::shared_ptr<UnboundPredicate>> TruncateArray(


I know Java impl uses the same name. However, I am still confused that it is generic enough to be used by transform other than truncate and it has nothing to do with array.

I rename it with TruncateByteArray, it's used for string and binary truncation. I also add a GenericTransform which is used as a fallback for any special cases.

wgtmac · 2025-12-01T10:02:24Z

src/iceberg/util/projection_util_internal.h

+              auto transformed,
+              func->Transform(literal.type()->type_id() == TypeId::kDate
+                                  ? Literal::Date(std::get<T>(literal.value()) - 1)
+                                  : Literal::Int(std::get<T>(literal.value()) - 1)));


This does not look safe since predicate may be of any type but TruncateInteger indicates that only integer variants are accepted.

I split the temporal usage into another function TransformTemporal.

src/iceberg/transform.h

src/iceberg/util/projection_util_internal.h

wgtmac · 2025-12-02T08:59:16Z

src/iceberg/util/projection_util_internal.h

+          default:
+            return GenericTransform(std::move(ref), predicate, func);
+        }
+        std::unreachable();


Why there is a std::unreachable() but other branches not?

wgtmac · 2025-12-02T09:01:49Z

src/iceberg/util/projection_util_internal.h

+
+  // Fixes an inclusive projection to account for incorrectly transformed values.
+  // align with Java implementation:
+  // https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/transforms/ProjectionUtil.java#L275


The link might be invalid over time because it points to the main branch.

Fixed by changing to a permanent link.

wgtmac · 2025-12-02T09:29:52Z

src/iceberg/util/projection_util_internal.h

+        return pred;
+      }
+    }
+    std::unreachable();


Should it be moved out of the switch block?

It's already out of the switch block.

wgtmac · 2025-12-02T09:31:25Z

src/iceberg/util/projection_util_internal.h

+      }
+    }
+
+    std::unreachable();


Should we return nullptr for consistency?

We can't reach here, so the unreachable should be fine.

wgtmac · 2025-12-02T09:47:28Z

src/iceberg/util/projection_util_internal.h

+    const auto width = truncate_transform->width();
+    ICEBERG_ASSIGN_OR_RAISE(auto ref, NamedReference::Make(std::string(name)));
+
+    if (str_value.length() < width) {


Is this correct? str_value.length() is the number of bytes not the number of UTF-8 chars but width is the number of UTF-8 chars.

Good catch, we should consider UTF-8, will fix.

Fixed by adding a CodePointCount function to do the count.

Add Transform::Project for inclusive predicate projection This PR implements Transform::Project, which transforms a BoundPredicate to an inclusive predicate on partition values. StrictProject will be added in a separate PR to keep the review easier. Move template implementations of Expressions into header file, or the linker will shout out the following: Undefined symbols for architecture x86_64: "std::__1::shared_ptr<iceberg::UnboundPredicateImpl<iceberg::BoundReference>> iceberg::Expressions::In<iceberg::BoundReference>(std::__1::shared_ptr<iceberg::UnboundTerm<iceberg::BoundReference>>, std::initializer_list<iceberg::Literal>)", referenced from: iceberg::ProjectionUtil::FixInclusiveTimeProjection(std::__1::shared_ptr<iceberg::UnboundPredicateImpl<iceberg::BoundReference>> const&) in transform.cc.o

wgtmac · 2025-12-03T02:33:02Z

src/iceberg/test/transform_test.cc

+  // The predicate's term should be a transform
+  EXPECT_EQ(bound_pred->term()->kind(), Term::Kind::kTransform);
+
+  auto dummy = Expressions::NotEqual<BoundTransform>(bucket_term, Literal::Int(5));


removed, sorry about the oversight :)

shangxinli reviewed Nov 29, 2025

View reviewed changes

src/iceberg/util/projection_util_internal.h Outdated Show resolved Hide resolved

wgtmac reviewed Dec 1, 2025

View reviewed changes

wgtmac reviewed Dec 2, 2025

View reviewed changes

zhjwpku added 6 commits December 3, 2025 08:05

fix: TruncateInteger should consider Literal type

ad56d17

fix: review comments

a19bb79

fix: use std::unique_ptr

da483bd

fix: review comments

7ec8a91

fix: change to a permanent link

5b9ddae

zhjwpku force-pushed the transform_project branch from 7a97b2e to 5b9ddae Compare December 3, 2025 00:09

wgtmac approved these changes Dec 3, 2025

View reviewed changes

fix: remove a unused statement

6456788

wgtmac merged commit 61b83ea into apache:main Dec 3, 2025
10 checks passed

zhjwpku deleted the transform_project branch December 3, 2025 15:23

feat: add transform project #371

feat: add transform project #371

Uh oh!

Conversation

zhjwpku commented Nov 29, 2025

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhjwpku Dec 2, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zhjwpku Dec 2, 2025 •

edited

Loading