feat: add visit type support #94
Conversation
Signed-off-by: Junwang Zhao <[email protected]>
Signed-off-by: Junwang Zhao <[email protected]>
Signed-off-by: Junwang Zhao <[email protected]>
| /// Unlike VisitTypeInline which calls `visitor.Visit`, here `visitor` | ||
| /// itself is called. | ||
| /// `visitor` must support a `const Type&` argument as a fallback, | ||
| /// in addition to concrete type classes. | ||
| /// | ||
| /// The intent is for this to be called on a generic lambda | ||
| /// that may internally use `if constexpr` or similar constructs. | ||
| template <typename VISITOR, typename... ARGS> | ||
| inline auto VisitType(const Type& type, VISITOR&& visitor, ARGS&&... args) | ||
| -> decltype(std::forward<VISITOR>(visitor)(type, args...)) { | ||
| switch (type.type_id()) { | ||
| ICEBERG_GENERATE_FOR_ALL_TYPES(TYPE_VISIT_INLINE); | ||
| default: | ||
| internal::Unreachable("Type not implemented"); |
There was a problem hiding this comment.
The docstring says a fallback is called but the body aborts in the default case?
There was a problem hiding this comment.
TBH I have no idea what that sentence actually means, I don't think these words does any help to understand the template, so I'd rather delete that, or do you have any suggestions?
There was a problem hiding this comment.
So this was copied from Arrow? It may be worth noting that in the header just for reference.
Looking at it again, I think what it means is that because the visitor here does not inherit from a base class that has all cases defined, you must make sure to handle all cases. So I guess the implementation is OK. But the wording could be improved.
There was a problem hiding this comment.
So this was copied from Arrow? It may be worth noting that in the header just for reference.
Yeah, it's adapted from arrow, I'll add the note.
Looking at it again, I think what it means is that because the visitor here does not inherit from a base class that has all cases defined, you must make sure to handle all cases. So I guess the implementation is OK. But the wording could be improved.
How about we keep this as is for now, and revisit it later, either improve the wording ourselves or sync with Arrow if they update their version.
The visit type infrastructure is adapted from apache arrow. Add a reference note per @lidavidm's suggestion. Signed-off-by: Junwang Zhao <[email protected]>
|
|
||
| namespace iceberg::internal { | ||
|
|
||
| [[noreturn]] ICEBERG_EXPORT void Unreachable( |
There was a problem hiding this comment.
Why not directly use std::unreachable?
There was a problem hiding this comment.
I did a quick search and found that std::unreachable was introduced in C++23 [1]. And it doesn't support passing a custom message, which I think might be useful for logging purposes.
There was a problem hiding this comment.
Ah my bad. Please ignore this comment.
| * under the License. | ||
| */ | ||
|
|
||
| #pragma once |
There was a problem hiding this comment.
nit: add the \file docstring and mention that it is adapted from Apache Arrow.
There was a problem hiding this comment.
Will add later today, thanks for the suggestion.
There was a problem hiding this comment.
If we take big pieces from other projects, then we also want to add that to the LICENSE:
For example in PyIceberg: https://github.com/apache/iceberg-python/blob/260ef54e3920d435ae3b2ccda090e66f9c1ac015/LICENSE#L206-L213
Add a
TypeIdfield to each concrete type to enable macro-based type visitor support.