pitrou commented on code in PR #47922:
URL: https://github.com/apache/arrow/pull/47922#discussion_r2759232268
##########
cpp/src/arrow/type_traits.cc:
##########
@@ -17,10 +17,11 @@
#include "arrow/type_traits.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type.h"
#include "arrow/util/logging_internal.h"
-namespace arrow {
Review Comment:
Why this change?
##########
cpp/src/arrow/type_traits.cc:
##########
@@ -17,10 +17,11 @@
#include "arrow/type_traits.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
+#include "arrow/type.h"
Review Comment:
Please remove these additions.
##########
cpp/src/arrow/type_traits.h:
##########
@@ -596,6 +598,17 @@ struct TypeTraits<ExtensionType> {
};
/// @}
+/// \brief Create a data type instance from a type ID for parameter-free types
Review Comment:
Hmm, why did you leave this here?
##########
cpp/src/arrow/type.cc:
##########
@@ -3552,4 +3553,26 @@ const std::vector<Type::type>& DecimalTypeIds() {
return type_ids;
}
+Result<std::shared_ptr<DataType>> type_singleton(Type::type id) {
+ struct Visitor {
+ Result<std::shared_ptr<DataType>> result;
+
+ template <typename T>
+ Status Visit(const T* type) {
+ if constexpr (TypeTraits<T>::is_parameter_free) {
+ result = TypeTraits<T>::type_singleton();
+ return Status::OK();
+ }
+ return Status::Invalid("Type ", T::type_id, " is not a parameter-free
singleton type.");
+ }
+ };
+
+ Visitor visitor;
+ auto status = VisitTypeIdInline(id, &visitor);
+ if (!status.ok()) {
Review Comment:
No need for this as the visitor is *already* returning a Status.
##########
cpp/src/arrow/type.cc:
##########
@@ -3552,4 +3553,26 @@ const std::vector<Type::type>& DecimalTypeIds() {
return type_ids;
}
+Result<std::shared_ptr<DataType>> type_singleton(Type::type id) {
+ struct Visitor {
+ Result<std::shared_ptr<DataType>> result;
+
+ template <typename T>
+ Status Visit(const T* type) {
+ if constexpr (TypeTraits<T>::is_parameter_free) {
+ result = TypeTraits<T>::type_singleton();
+ return Status::OK();
+ }
+ return Status::Invalid("Type ", T::type_id, " is not a parameter-free
singleton type.");
Review Comment:
As I suggested before, please return TypeError.
##########
cpp/src/arrow/type_test.cc:
##########
@@ -50,6 +52,53 @@ TEST(TestTypeId, AllTypeIds) {
ASSERT_EQ(static_cast<int>(all_ids.size()), Type::MAX_ID);
}
+TEST(TestTypeSingleton, ParameterFreeTypes) {
+ // Test successful cases - parameter-free types (sample a few)
+ std::vector<std::pair<Type::type, std::shared_ptr<DataType>>> cases = {
+ {Type::NA, null()},
+ {Type::BOOL, boolean()},
+ {Type::INT32, int32()},
+ {Type::STRING, utf8()},
+ {Type::DATE32, date32()},
+ };
+
+ for (const auto& test_case : cases) {
+ SCOPED_TRACE("Testing type: " +
std::to_string(static_cast<int>(test_case.first)));
+ auto result = type_singleton(test_case.first);
+ ASSERT_OK_AND_ASSIGN(auto type, result);
+ ASSERT_TRUE(type->Equals(*test_case.second))
+ << "Failed on type " << test_case.first << ". Expected "
+ << test_case.second->ToString() << " but got " << type->ToString();
+ }
+}
+
+TEST(TestTypeSingleton, ParameterizedTypes) {
+ // Test error cases - parameterized types
+ std::vector<Type::type> parameterized_types = {
Review Comment:
Same comment here: no need to test these exhaustively, only one of them will
be enough.
--
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]
