-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-14615: [C++] Refactor nested field refs and add union support #11641
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,6 +223,18 @@ class ARROW_EXPORT SetLookupOptions : public FunctionOptions { | |
| bool skip_nulls; | ||
| }; | ||
|
|
||
| /// Options for struct_field function | ||
| class ARROW_EXPORT StructFieldOptions : public FunctionOptions { | ||
| public: | ||
| explicit StructFieldOptions(std::vector<int> indices); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder whether this should also accept a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FieldRef is relative to a schema so we'd want/need a variant of this function that operates on a RecordBatch.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But a plain string name could be useful for a StructArray?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd still need a type to resolve it to an index, right? Unless you mean storing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, storing it as strings on the options, I meant. Because when actually executing the kernel, the struct array itself can perfectly resolve the name I think?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. Want to file a followup? I think we can support having a FieldRef internally, basically. (Though the interpretation will be a little different - it'll be relative to an array, not a schema.)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that I think of it, it's probably better to resolve the field up front (using the schema) than pay the cost for every kernel invocation with the same schema. |
||
| StructFieldOptions(); | ||
| constexpr static char const kTypeName[] = "StructFieldOptions"; | ||
|
|
||
| /// The child indices to extract. For instance, to get the 2nd child | ||
| /// of the 1st child of a struct or union, this would be {0, 1}. | ||
| std::vector<int> indices; | ||
| }; | ||
|
|
||
| class ARROW_EXPORT StrptimeOptions : public FunctionOptions { | ||
| public: | ||
| explicit StrptimeOptions(std::string format, TimeUnit::type unit); | ||
|
|
||
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.
Also test with an empty union array?