pitrou commented on code in PR #47922:
URL: https://github.com/apache/arrow/pull/47922#discussion_r2769349020
##########
cpp/src/arrow/type_test.cc:
##########
@@ -50,6 +52,41 @@ 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();
Review Comment:
Suggested improvement: you can use `AssertTypeEqual` to automate the
generation of a nice error message if the types don't match.
##########
cpp/src/arrow/type_test.cc:
##########
@@ -50,6 +52,41 @@ 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 (test one representative)
+ auto result = type_singleton(Type::TIMESTAMP);
+ ASSERT_FALSE(result.ok());
+ EXPECT_THAT(result.status().message(),
+ testing::HasSubstr("is not a parameter-free singleton type"));
+}
+
+TEST(TestTypeSingleton, InvalidType) {
Review Comment:
I don't think this test is useful, passing an invalid type id is a
programming error.
##########
cpp/src/arrow/type_traits.h:
##########
@@ -22,8 +22,10 @@
#include <type_traits>
#include <vector>
+#include "arrow/result.h"
Review Comment:
Why these additions? Let's remove them.
##########
cpp/src/arrow/type_test.cc:
##########
@@ -33,12 +33,14 @@
#include "arrow/memory_pool.h"
#include "arrow/table.h"
#include "arrow/testing/gtest_util.h"
+#include "arrow/testing/matchers.h"
#include "arrow/testing/random.h"
#include "arrow/testing/util.h"
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/key_value_metadata.h"
+#include "arrow/util/logging.h"
Review Comment:
This include doesn't seem actually used?
##########
cpp/src/arrow/type.cc:
##########
@@ -3552,4 +3553,23 @@ 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::TypeError("Type ", T::type_id, " is not a parameter-free
singleton type.");
Review Comment:
Small wording nit to make the error message more readable:
```suggestion
return Status::TypeError("Type ", ToString(T::type_id), " is not a
parameter-free type");
```
##########
cpp/src/arrow/type_traits.cc:
##########
@@ -16,11 +16,9 @@
// under the License.
#include "arrow/type_traits.h"
-
+#include "arrow/type.h"
Review Comment:
Not sure why this addition is required?
##########
cpp/src/arrow/type_test.cc:
##########
@@ -50,6 +52,41 @@ 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 (test one representative)
+ auto result = type_singleton(Type::TIMESTAMP);
+ ASSERT_FALSE(result.ok());
Review Comment:
```suggestion
ASSERT_RAISES(TypeError, result);
```
##########
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:
You still forgot to undo this.
##########
cpp/src/arrow/type.h:
##########
@@ -2645,4 +2645,17 @@ const std::vector<std::shared_ptr<DataType>>&
PrimitiveTypes();
ARROW_EXPORT
const std::vector<Type::type>& DecimalTypeIds();
+/// \brief Create a data type instance from a type ID for parameter-free types
+///
+/// This function creates a data type instance for types that don't require
+/// additional parameters (where TypeTraits<T>::is_parameter_free is true).
+/// For types that require parameters (like TimestampType or ListType),
+/// this function will return an error.
+///
+/// \param[in] id The type ID to create a type instance for
+/// \return Result with a shared pointer to the created DataType instance,
+/// or an error if the type requires parameters
Review Comment:
```suggestion
/// \param[in] id The type ID to create a type instance for
/// \return The type instance for the given type ID,
/// or a TypeError if the type requires parameters
```
--
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]
