Distinguish null constant and non-null constant in simple function's initialize method#10638
Distinguish null constant and non-null constant in simple function's initialize method#10638philo-he wants to merge 4 commits intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
ae3a2df to
9bd80b1
Compare
|
@mbasmanova, this is just an initial proposal to fix the mentioned issue. Could you take a look? |
9bd80b1 to
8fd1d31
Compare
There was a problem hiding this comment.
@philo-he Thank you for working on this. Would you show an example of a how 'initialize' API would change? Also, wondering if it would make sense to use exec::optional_arg_type<T> instead of this new class.
There was a problem hiding this comment.
Would you show an example of a how 'initialize' API would change?
@mbasmanova, I just updated this pr. You can see all intialize methods are affected due to its arg type is changed.
wondering if it would make sense to use exec::optional_arg_type instead of this new class.
Yes, this existing type is enough. Have tried using it in the latest code. Could you take a high-level review firstly?
There was a problem hiding this comment.
Why accessor_arg_type ? Can we not use optional_arg_type ?
mbasmanova
left a comment
There was a problem hiding this comment.
@philo-he Thank you for working on this. This should work % the name. I'm out next week.
There was a problem hiding this comment.
Let's rename to optional_arg_type . See velox/exec/SimpleAggregateAdapter.h
// The reader type of T used in simple UDAF interface. An instance of
// arg_type allows reading one row from the input vector. This is used for UDAFs
// that have non-default null behavior.
template <typename T>
using optional_arg_type = OptionalAccessor<T>;
0378252 to
f2369a8
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions! |
f2369a8 to
ef44db4
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions! |
This reverts commit 9fcf843b6a9840b526f7294af8d0fd0fe2225e52.
In the current simple function framework, if null constant argument is passed, initialize method will wrongly use the value 0 of NullConstant vector. In this pr,
ConstantArgis proposed to represent function's input with null constant and non-null constant distinguished. If the proposed API change is acceptable, this pr will modify all simple functions' initialize method.Fix #6569.