misc(examples): Extend simple functions example to use nested complex types #12061
Conversation
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D68047322 |
✅ Deploy Preview for meta-velox canceled.
|
| struct MyNestedComplexTimesTwoFunction { | ||
| VELOX_DEFINE_FUNCTION_TYPES(TExecParams); | ||
|
|
||
| FOLLY_ALWAYS_INLINE bool call( |
There was a problem hiding this comment.
can you add a comment here to describe the exact input and output types? Just to ensure folks without a deeper understand can still follow what is going on.
There was a problem hiding this comment.
Good call. Added comments here and across all of the new code as a walkthrough. PTAL
| const arg_type<Array<Map<int64_t, double>>>& inputArrayOfMap) { | ||
| result.reserve(inputArrayOfMap.size()); | ||
| for (const auto& innerMap : inputArrayOfMap) { | ||
| if (!innerMap.has_value()) { |
There was a problem hiding this comment.
just in case, please comment the body as well. This is not "real" code, so the more we walk the reader through what is going on, the better.
… types (facebookincubator#12061) Summary: 1. Added a new set of UDFs w/ input, output args using nested complex types between array, map and row. 2. Changed the required struct level template arg from 'T' to 'TExecParams' as a best practice Differential Revision: D68047322
kKPulla
force-pushed
the
export-D68047322
branch
from
51b753e to
c5be800
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68047322 |
| // This method takes and returns an Array of Maps. The return value has the | ||
| // values of the inner map doubled. Both array and map proxy objects are | ||
| // currently implemented based on std::vector and std::unordered_map, | ||
| // respectively. Vector elements and map values are currently wrapped by |
There was a problem hiding this comment.
Both array and map proxy objects are currently implemented based on std::vector and std::unordered_map, respectively.
What do you mean by this? It sounds like they are internally backed by std containers, which is not the case. Maybe rephrase to make it clearer that they are implemented in such as way as to follow the std API as much as possible, but in come cases this can't be done (but that they are not in fact based on slow std containers)
There was a problem hiding this comment.
I copied this comment from the existing plustwo functions above which said the same about implementing based on std:: containers. I agree it can be misleading, can rephrase that too if we agree.
| continue; | ||
| } | ||
| for (const auto& val : arrayVal.value()) { | ||
| valueWriter.add_item() = val.has_value() ? val.value() * 2 : 0; |
There was a problem hiding this comment.
nit: if an array element is null, shouldn't the output (NULL*2) also be NULL?
There was a problem hiding this comment.
True :) I kept the output null free, again matching with the plustwo functions above. No strong preference either way.
Summary:
Differential Revision: D68047322