sql: move NullableArgs function property to overload level #83732
Conversation
|
This change is |
chengxiong-ruan
changed the title
chengxiong-ruan
force-pushed
the
move-nullableargs-to-overload-level
branch
3 times, most recently
from
5d3a5ab to
fd25e76
Compare
chengxiong-ruan
changed the title
|
CI failures seems not relevant |
| ) | ||
| } | ||
|
|
||
| func makeDefaultAggOverloadWithReturnType( | ||
| in []*types.T, retType tree.ReturnTyper, f eval.AggregateOverload, info string, |
There was a problem hiding this comment.
This is starting to feel like enough arguments that it'd be more readable to make a struct
There was a problem hiding this comment.
Feel free to not act on this one.
pkg/sql/sem/tree/type_check.go
Outdated
| @@ -1071,17 +1071,27 @@ func (expr *FuncExpr) TypeCheck( | |||
| return nil, pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s()", def.Name) | |||
| } | |||
|
|
|||
| var nullableArgFns []overloadImpl | |||
| var unNullableArgFns []overloadImpl | |||
There was a problem hiding this comment.
What about notNullableArgsFn?
pkg/sql/sem/tree/type_check.go
Outdated
| if f.(*Overload).NullableArgs { | ||
| nullableArgFns = append(nullableArgFns, f) | ||
| continue | ||
| } | ||
| unNullableArgFns = append(unNullableArgFns, f) |
There was a problem hiding this comment.
nit: this seems like a nice place for an else
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @chengxiong-ruan, and @rafiss)
pkg/sql/sem/builtins/aggregate_builtins.go line 654 at r1 (raw file):
return b } func makeDefaultAggOverload(
should we name this makeImmutableAggOverload (and ditto below)?
chengxiong-ruan
force-pushed
the
move-nullableargs-to-overload-level
branch
from
fd25e76 to
dceae64
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/builtins/aggregate_builtins.go line 654 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
should we name this
makeImmutableAggOverload(and ditto below)?
yes, good point, changed to that.
pkg/sql/sem/tree/type_check.go line 1075 at r1 (raw file):
Previously, ajwerner wrote…
What about
notNullableArgsFn?
done, sorry for the bad naming
pkg/sql/sem/tree/type_check.go line 1081 at r1 (raw file):
Previously, ajwerner wrote…
nit: this seems like a nice place for an
else
done
Currently we only have builtins, and that all overloads of a same function name share function properties. However, we're going to support user defined functions whose properties can vary as how users define them. So we need to move any property that's relevant to UDF to overload level. Currently, `NullableArgs` is the only one matters. Release note: None.
chengxiong-ruan
force-pushed
the
move-nullableargs-to-overload-level
branch
from
dceae64 to
7e6d300
Compare
|
TFTR! |
|
Build succeeded: |
Currently we only have builtins, and that all overloads of a
same function name share function properties. However, we're
going to support user defined functions whose properties can
vary as how users define them. So we need to move any property
that's relevant to UDF to overload level. Currently,
NullableArgsis the only one matters.
Release note: None.