lidavidm commented on code in PR #13567:
URL: https://github.com/apache/arrow/pull/13567#discussion_r917904533
##########
cpp/src/arrow/dataset/partition.h:
##########
@@ -310,6 +322,11 @@ class ARROW_DS_EXPORT FunctionPartitioning : public
Partitioning {
std::string type_name() const override { return name_; }
+ bool Equals(const Partitioning& other, bool check_metadata) const override {
+ return ::arrow::internal::checked_cast<const FunctionPartitioning&>(other)
+ .type_name() == type_name();
Review Comment:
TBH, I'm not sure it makes sense to implement Equals for
FunctionPartitioning. Or else at best we should check `this == &other`. Also
why is this inline in the header unlike the others?
##########
cpp/src/arrow/dataset/partition.cc:
##########
@@ -381,6 +404,15 @@ Result<std::vector<KeyValuePartitioning::Key>>
DirectoryPartitioning::ParseKeys(
return ParsePartitionSegments(segments);
}
+bool DirectoryPartitioning::Equals(const Partitioning& other, bool
check_metadata) const {
+ if (this == &other) {
+ return true;
+ }
+ const auto& direct_part =
+ ::arrow::internal::checked_cast<const DirectoryPartitioning&>(other);
+ return type_name() == direct_part.type_name() &&
KeyValuePartitioning::Equals(other);
Review Comment:
```suggestion
return type_name() == direct_part.type_name() &&
KeyValuePartitioning::Equals(other);
```
##########
cpp/src/arrow/dataset/partition.cc:
##########
@@ -82,6 +82,14 @@ std::shared_ptr<Partitioning> Partitioning::Default() {
std::string type_name() const override { return "default"; }
+ bool Equals(const Partitioning& other, bool check_metadata) const override
{
+ if (this == &other) {
+ return true;
+ }
+ return checked_cast<const DefaultPartitioning&>(other).type_name() ==
type_name() &&
Review Comment:
Don't do the checked cast here, it'll be wrong if the other partitioning
isn't the same type. You don't use any methods from the derived type anyways.
##########
cpp/src/arrow/dataset/partition.cc:
##########
@@ -381,6 +404,15 @@ Result<std::vector<KeyValuePartitioning::Key>>
DirectoryPartitioning::ParseKeys(
return ParsePartitionSegments(segments);
}
+bool DirectoryPartitioning::Equals(const Partitioning& other, bool
check_metadata) const {
+ if (this == &other) {
+ return true;
+ }
+ const auto& direct_part =
+ ::arrow::internal::checked_cast<const DirectoryPartitioning&>(other);
+ return type_name() == direct_part.type_name() &&
KeyValuePartitioning::Equals(other);
Review Comment:
and ditto below (since KeyValuePartitioning::Equals does the `this ==
&other` check already
##########
cpp/src/arrow/dataset/partition_test.cc:
##########
@@ -206,6 +206,15 @@ TEST_F(TestPartitioning, DirectoryPartitioning) {
equal(field_ref("beta"),
literal("foo"))));
}
Review Comment:
I'd like to see tests that partitionings of different types don't compare
equal.
##########
cpp/src/arrow/dataset/partition.cc:
##########
@@ -754,6 +794,17 @@ Result<PartitionPathFormat> HivePartitioning::FormatValues(
return
PartitionPathFormat{fs::internal::JoinAbstractPath(std::move(segments)), ""};
}
+bool HivePartitioning::Equals(const Partitioning& other, bool check_metadata)
const {
+ if (this == &other) {
+ return true;
+ }
+ const auto& hive_part = ::arrow::internal::checked_cast<const
HivePartitioning&>(other);
+ return type_name() == hive_part.type_name() &&
Review Comment:
Do the type name check first, then cast and do the rest of the comparisons.
--
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]
