-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enum Functions #2793
Enum Functions #2793
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Looks good. Some comments:
src/common/types.cpp
Outdated
idx_t EnumType::GetValuePosition(const Value &val) { | ||
switch (val.type().InternalType()) { | ||
case PhysicalType::UINT8: | ||
return val.value_.utinyint; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val.GetValue<uint32_t>()
should cover all these three cases so the switch is no longer required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't but i've moved it there.
} | ||
|
||
void EnumFirst::RegisterFunction(BuiltinFunctions &set) { | ||
set.AddFunction(ScalarFunction("enum_first", {LogicalType::ANY}, LogicalType::VARCHAR, EnumFirstFunction, false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you have a binder, but maybe ANY
should be ENUM
? I'm not sure how the candidate functions will be displayed in the situation where there are too many or too few arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that the creation and equality of enum types are associated with the enum categories. I guess we could to these matches on LogicalTypeIDs, but I think @Mytherin prefers these binding setups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binding on an empty type is how the list functions do this, e.g. unnest binds on {LogicalTypeId::LIST}
. Perhaps this should indeed bind on LogicalTypeId::ENUM
instead of ANY
.
Thanks! |
Issue: #2741 and #2740