GH-46531: [C++] Add type_singleton utility function and tests.#47922
GH-46531: [C++] Add type_singleton utility function and tests.#47922pitrou merged 1 commit intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
|
@pitrou Cl jobs are awaiting maintainer approval. Please approve to trigger the remaining checks. |
|
@pitrou |
|
|
pitrou
left a comment
There was a problem hiding this comment.
Thanks for doing this @harshkumar-2005 . Please find a couple comments below.
a171977 to
98795b2
Compare
|
@harshkumar-2005 Are you using AI for this PR? |
cpp/src/arrow/type_test.cc
Outdated
|
|
||
| TEST(TestTypeSingleton, ParameterizedTypes) { | ||
| // Test error cases - parameterized types | ||
| std::vector<Type::type> parameterized_types = { |
There was a problem hiding this comment.
Same comment here: no need to test these exhaustively, only one of them will be enough.
| #include "arrow/type.h" | ||
| #include "arrow/util/logging_internal.h" | ||
|
|
||
| namespace arrow { |
cpp/src/arrow/type.cc
Outdated
|
|
||
| Visitor visitor; | ||
| auto status = VisitTypeIdInline(id, &visitor); | ||
| if (!status.ok()) { |
There was a problem hiding this comment.
No need for this as the visitor is already returning a Status.
Yes, partially—for understanding a few concepts. The implementation and final code are mine. |
Ok, thanks. I was asking because your latest changes seemed unfinished and don't compile at all on CI. |
pitrou
left a comment
There was a problem hiding this comment.
@harshkumar-2005 Here are a couple more suggestions. Also, please review and build your own code before submitting, as some changes probably break compilation.
cpp/src/arrow/type_test.cc
Outdated
| ASSERT_TRUE(type->Equals(*test_case.second)) | ||
| << "Failed on type " << test_case.first << ". Expected " | ||
| << test_case.second->ToString() << " but got " << type->ToString(); |
There was a problem hiding this 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
Outdated
| testing::HasSubstr("is not a parameter-free singleton type")); | ||
| } | ||
|
|
||
| TEST(TestTypeSingleton, InvalidType) { |
There was a problem hiding this comment.
I don't think this test is useful, passing an invalid type id is a programming error.
cpp/src/arrow/type_test.cc
Outdated
| #include "arrow/type_traits.h" | ||
| #include "arrow/util/checked_cast.h" | ||
| #include "arrow/util/key_value_metadata.h" | ||
| #include "arrow/util/logging.h" |
There was a problem hiding this comment.
This include doesn't seem actually used?
| #include "arrow/type.h" | ||
| #include "arrow/util/logging_internal.h" | ||
|
|
||
| namespace arrow { |
cpp/src/arrow/type_traits.cc
Outdated
|
|
||
| #include "arrow/type_traits.h" | ||
|
|
||
| #include "arrow/type.h" |
There was a problem hiding this comment.
Not sure why this addition is required?
cpp/src/arrow/type_traits.h
Outdated
| #include <type_traits> | ||
| #include <vector> | ||
|
|
||
| #include "arrow/result.h" |
There was a problem hiding this comment.
Why these additions? Let's remove them.
|
|
1 similar comment
|
|
a10dfb1 to
4bb9a23
Compare
4bb9a23 to
0a87cce
Compare
|
I've pushed some updates to fix compilation and simplify the implementation. Will merge if green. |
|
After merging your PR, Conbench analyzed the 1 benchmarking run that has been run so far on merge-commit 99984fd. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 37 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Introduce a
type_singleton(Type::type id)utility to create parameter-free DataType instances (such as int32, boolean, utf8, etc.). Returns an error for parameterized types.Are these changes tested?
Yes, by additional unit tests in
type_test.cc.Are there any user-facing changes?
No.