westonpace commented on code in PR #13567:
URL: https://github.com/apache/arrow/pull/13567#discussion_r918002885
##########
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:
I agree with @lidavidm that a checked_cast should be avoided. If you
received a pointer type `Partitioning*` you could do a `dynamic_cast` instead
of the `type_name` comparison. However, a failed `dynamic_cast` is an
exception when you have a reference type and it's probably better to just
compare `type_name`.
The comparison to `type_name` is basically doing the same thing as a cast
check anyways.
The reason a `checked_cast` is a bad idea is that it should be ok for a user
to do:
```
DefaultPartitioning one;
KeyValuePartitioning two(schema);
if (one.Equals(two)) {
std::cout << "equal" << std::endl;
}
```
This `Equals` method should always return false so this toy example is
somewhat meaningless. However, it could be useful if the actual partitions
were based on file metadata or something dynamically calculated. If there is a
`checked_cast` then, instead of false, we will get an abort in debug mode and
potentially a segmentation fault in release mode.
--
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]
