pitrou commented on code in PR #39937:
URL: https://github.com/apache/arrow/pull/39937#discussion_r1478420891
##########
cpp/src/arrow/datum.cc:
##########
@@ -73,86 +74,108 @@ std::shared_ptr<Array> Datum::make_array() const {
}
const std::shared_ptr<DataType>& Datum::type() const {
- if (this->kind() == Datum::ARRAY) {
- return std::get<std::shared_ptr<ArrayData>>(this->value)->type;
- }
- if (this->kind() == Datum::CHUNKED_ARRAY) {
- return std::get<std::shared_ptr<ChunkedArray>>(this->value)->type();
- }
- if (this->kind() == Datum::SCALAR) {
- return std::get<std::shared_ptr<Scalar>>(this->value)->type;
- }
- static std::shared_ptr<DataType> no_type;
- return no_type;
+ return std::visit(
+ [](const auto& value) -> const std::shared_ptr<DataType>& {
+ using T = std::decay_t<decltype(value)>;
+
+ if constexpr (std::is_same_v<T, std::shared_ptr<Scalar>> ||
+ std::is_same_v<T, std::shared_ptr<ArrayData>>) {
+ return value->type;
+ } else if constexpr (std::is_same_v<T, std::shared_ptr<ChunkedArray>>)
{
+ return value->type();
+ } else {
+ static std::shared_ptr<DataType> no_type;
+ return no_type;
+ }
+ },
+ this->value);
}
const std::shared_ptr<Schema>& Datum::schema() const {
- if (this->kind() == Datum::RECORD_BATCH) {
- return std::get<std::shared_ptr<RecordBatch>>(this->value)->schema();
- }
- if (this->kind() == Datum::TABLE) {
- return std::get<std::shared_ptr<Table>>(this->value)->schema();
- }
- static std::shared_ptr<Schema> no_schema;
- return no_schema;
+ return std::visit(
+ [](const auto& value) -> const std::shared_ptr<Schema>& {
+ using T = std::decay_t<decltype(value)>;
+
+ if constexpr (std::is_same_v<T, std::shared_ptr<RecordBatch>> ||
+ std::is_same_v<T, std::shared_ptr<Table>>) {
+ return value->schema();
+ } else {
+ static std::shared_ptr<Schema> no_schema;
+ return no_schema;
Review Comment:
Same question here.
##########
cpp/src/arrow/datum.cc:
##########
@@ -73,86 +74,108 @@ std::shared_ptr<Array> Datum::make_array() const {
}
const std::shared_ptr<DataType>& Datum::type() const {
- if (this->kind() == Datum::ARRAY) {
- return std::get<std::shared_ptr<ArrayData>>(this->value)->type;
- }
- if (this->kind() == Datum::CHUNKED_ARRAY) {
- return std::get<std::shared_ptr<ChunkedArray>>(this->value)->type();
- }
- if (this->kind() == Datum::SCALAR) {
- return std::get<std::shared_ptr<Scalar>>(this->value)->type;
- }
- static std::shared_ptr<DataType> no_type;
- return no_type;
+ return std::visit(
+ [](const auto& value) -> const std::shared_ptr<DataType>& {
+ using T = std::decay_t<decltype(value)>;
+
+ if constexpr (std::is_same_v<T, std::shared_ptr<Scalar>> ||
+ std::is_same_v<T, std::shared_ptr<ArrayData>>) {
+ return value->type;
+ } else if constexpr (std::is_same_v<T, std::shared_ptr<ChunkedArray>>)
{
+ return value->type();
+ } else {
+ static std::shared_ptr<DataType> no_type;
+ return no_type;
Review Comment:
Does it make sense to keep a static variable for this?
```suggestion
return {};
```
##########
cpp/src/arrow/datum.h:
##########
@@ -170,22 +170,27 @@ struct ARROW_EXPORT Datum {
/// \brief The kind of data stored in Datum
Datum::Kind kind() const {
- switch (this->value.index()) {
- case 0:
- return Datum::NONE;
- case 1:
- return Datum::SCALAR;
- case 2:
- return Datum::ARRAY;
- case 3:
- return Datum::CHUNKED_ARRAY;
- case 4:
- return Datum::RECORD_BATCH;
- case 5:
- return Datum::TABLE;
- default:
- return Datum::NONE;
- }
+ return std::visit(
+ [](const auto& value) -> Datum::Kind {
+ using T = std::decay_t<decltype(value)>;
+
+ if constexpr (std::is_same_v<T, Empty>) {
+ return Datum::NONE;
+ } else if constexpr (std::is_same_v<T, std::shared_ptr<Scalar>>) {
+ return Datum::SCALAR;
+ } else if constexpr (std::is_same_v<T, std::shared_ptr<ArrayData>>) {
+ return Datum::ARRAY;
+ } else if constexpr (std::is_same_v<T,
std::shared_ptr<ChunkedArray>>) {
+ return Datum::CHUNKED_ARRAY;
+ } else if constexpr (std::is_same_v<T,
std::shared_ptr<RecordBatch>>) {
+ return Datum::RECORD_BATCH;
+ } else if constexpr (std::is_same_v<T, std::shared_ptr<Table>>) {
+ return Datum::TABLE;
+ } else {
+ static_assert(!std::is_same_v<T, T>, "unhandled type");
Review Comment:
Why not simply
```suggestion
static_assert(false, "unhandled type");
```
##########
cpp/src/arrow/datum.cc:
##########
@@ -20,6 +20,7 @@
#include <cstddef>
#include <memory>
#include <sstream>
+#include <variant>
Review Comment:
This shouldn't be required, should it?
--
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]
