wgtmac commented on code in PR #284:
URL: https://github.com/apache/iceberg-cpp/pull/284#discussion_r2486650259
##########
src/iceberg/transform.cc:
##########
@@ -124,6 +124,35 @@ Result<std::shared_ptr<TransformFunction>> Transform::Bind(
}
}
+bool Transform::PreservesOrder() const {
+ switch (transform_type_) {
+ case TransformType::kUnknown:
+ case TransformType::kVoid:
+ case TransformType::kBucket:
+ return false;
+ default:
Review Comment:
Should we explicitly list all `transform_type_`s just in case of forgetting
to add a new one in the future?
##########
src/iceberg/transform.cc:
##########
@@ -124,6 +124,35 @@ Result<std::shared_ptr<TransformFunction>> Transform::Bind(
}
}
+bool Transform::PreservesOrder() const {
+ switch (transform_type_) {
+ case TransformType::kUnknown:
+ case TransformType::kVoid:
+ case TransformType::kBucket:
+ return false;
+ default:
+ return true;
+ }
+}
+
+bool Transform::SatisfiesOrderOf(const Transform& other) const {
+ auto other_type = other.transform_type();
+ switch (transform_type_) {
+ case TransformType::kIdentity:
+ return other.PreservesOrder();
+ case TransformType::kHour:
+ return other_type == TransformType::kHour || other_type ==
TransformType::kDay ||
+ other_type == TransformType::kMonth || other_type ==
TransformType::kYear;
+ case TransformType::kDay:
+ return other_type == TransformType::kDay || other_type ==
TransformType::kMonth ||
+ other_type == TransformType::kYear;
+ case TransformType::kMonth:
+ return other_type == TransformType::kMonth || other_type ==
TransformType::kYear;
+ default:
Review Comment:
ditto
##########
src/iceberg/transform.cc:
##########
@@ -124,6 +124,35 @@ Result<std::shared_ptr<TransformFunction>> Transform::Bind(
}
}
+bool Transform::PreservesOrder() const {
+ switch (transform_type_) {
+ case TransformType::kUnknown:
+ case TransformType::kVoid:
+ case TransformType::kBucket:
+ return false;
+ default:
+ return true;
+ }
+}
+
+bool Transform::SatisfiesOrderOf(const Transform& other) const {
+ auto other_type = other.transform_type();
+ switch (transform_type_) {
+ case TransformType::kIdentity:
+ return other.PreservesOrder();
Review Comment:
```suggestion
// ordering by value is the same as long as the other preserves order
return other.PreservesOrder();
```
##########
src/iceberg/transform.cc:
##########
@@ -124,6 +124,35 @@ Result<std::shared_ptr<TransformFunction>> Transform::Bind(
}
}
+bool Transform::PreservesOrder() const {
+ switch (transform_type_) {
+ case TransformType::kUnknown:
+ case TransformType::kVoid:
+ case TransformType::kBucket:
+ return false;
+ default:
+ return true;
+ }
+}
+
+bool Transform::SatisfiesOrderOf(const Transform& other) const {
+ auto other_type = other.transform_type();
+ switch (transform_type_) {
+ case TransformType::kIdentity:
+ return other.PreservesOrder();
+ case TransformType::kHour:
+ return other_type == TransformType::kHour || other_type ==
TransformType::kDay ||
+ other_type == TransformType::kMonth || other_type ==
TransformType::kYear;
+ case TransformType::kDay:
+ return other_type == TransformType::kDay || other_type ==
TransformType::kMonth ||
+ other_type == TransformType::kYear;
+ case TransformType::kMonth:
+ return other_type == TransformType::kMonth || other_type ==
TransformType::kYear;
+ default:
Review Comment:
BTW, it seems that this implementation is incomplete? For example, truncate
should have special logic.
##########
src/iceberg/sort_order.h:
##########
@@ -49,6 +49,20 @@ class ICEBERG_EXPORT SortOrder : public util::Formattable {
/// \brief Get the list of sort fields.
std::span<const SortField> fields() const;
+ /// \brief Returns true if the sort order is sorted
+ bool IsSorted() const { return !fields_.empty(); }
+
+ /// \brief Returns true if the sort order is unsorted
+ /// A SortOrder is unsorted if it has no sort fields.
+ bool IsUnsorted() const { return fields_.empty(); }
Review Comment:
```suggestion
bool is_unsorted() const { return fields_.empty(); }
```
##########
src/iceberg/sort_order.h:
##########
@@ -49,6 +49,20 @@ class ICEBERG_EXPORT SortOrder : public util::Formattable {
/// \brief Get the list of sort fields.
std::span<const SortField> fields() const;
+ /// \brief Returns true if the sort order is sorted
+ bool IsSorted() const { return !fields_.empty(); }
Review Comment:
```suggestion
bool is_sorted() const { return !fields_.empty(); }
```
##########
src/iceberg/sort_order.cc:
##########
@@ -38,10 +39,48 @@ int32_t SortOrder::order_id() const { return order_id_; }
std::span<const SortField> SortOrder::fields() const { return fields_; }
+bool SortOrder::Satisfies(const SortOrder& other) const {
+ // any ordering satisfies an unsorted ordering
+ if (other.IsUnsorted()) {
+ return true;
+ }
+
+ // this ordering cannot satisfy an ordering with more sort fields
+ if (fields_.size() < other.fields().size()) {
+ return false;
+ }
+
+ // this ordering has either more or the same number of sort fields
+ for (const auto& [field, other_field] : std::views::zip(fields_,
other.fields_)) {
+ if (!field.Satisfies(other_field)) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+bool SortOrder::SameOrder(const SortOrder& other) const {
+ if (fields_.size() != other.fields().size()) {
+ return false;
+ }
+
+ for (const auto& [field, other_field] : std::views::zip(fields_,
other.fields())) {
+ if (!(field == other_field)) {
+ return false;
+ }
+ }
+
+ return true;
Review Comment:
```suggestion
return fields_ == other.fields_;
```
Can we simplify it as above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
