Add Tuple Element Names to TupleType#491
Add Tuple Element Names to TupleType#491IyeOnline wants to merge 3 commits intoClickHouse:masterfrom
TupleType#491Conversation
This extends the type parser to support an element name alongside the element type and wires this through to make the field names in a tuple available in the C++ API.
slabko
left a comment
slabko
left a comment
There was a problem hiding this comment.
Thank you very much for your PR. This absolutely makes sense and is well implemented. I’ve added a couple of points; they’re not critical, so feel free to ignore them if you don’t have time to address them. You can go ahead and merge, and I’ll take care of the remaining required changes.
| /// Name of this element inside its parent (e.g. field name inside a named | ||
| /// Tuple). Empty for unnamed elements. | ||
| std::string element_name; |
There was a problem hiding this comment.
I think it would make more sense to add a vector here, called all_names, store both the name and the type in it, and keep the name field set to the last value as it currently works.
/// Type's name.
/// Need to cache TypeAst, so can't use StringView for name.
std::string name;
/// List of all names assigned to the element, for example
/// named tuples, have name and type, `Tuple(i Int64, s String)`.
std::vector<std::string> all_names
There was a problem hiding this comment.
Personally I dont really like that, because it
- Creates two spots containing the same information
- Does not make it clear through the type-system/name that the entries in
all_namesare rather different entities (field name, vs type name)
|
|
||
| /// Tuple types can be appended if they have the same shape and | ||
| /// the to-be-appended type has the same names or is entirely unnamed. | ||
| static bool CanAppendType(const TypeRef& destination_type, const TypeRef& source_type) { |
There was a problem hiding this comment.
Not a big deal at all, but it might be worth considering making this a member function. It could help avoid any confusion between the source and destination, and would keep the interface to a single parameter - the type to append. Also it looks like the project tends to use anonymous namespaces rather than static for internal linkage.
There was a problem hiding this comment.
The problem is that we need to support recursion, so unless we extend the generic Type or TupleType interface, this cant work.
I don't want to introduce a concept of "type similarity" in this PR, so I think its best to just stay with the free function.
IyeOnline
force-pushed
the
topic/tuple-names
branch
from
9ebfc62 to
84e8caa
Compare
IyeOnline
force-pushed
the
topic/tuple-names
branch
from
84e8caa to
e6758c1
Compare
2 participants
This extends the type parser to support an element name alongside the
element type and wires this through to make the field names in
TupleTypeavailable in the C++ API.