Unary Complement execution has different results when the parameters are different #76943
Conversation
|
This change is |
|
the tests that fail will be interesting o_o can you comment on why commenting that out fixed things? |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/sem/tree/overload.go, line 746 at r1 (raw file):
}) } if ok, typedExprs, fn, err := checkReturn(ctx, semaCtx, &s); ok {
checkReturn was short circuiting for '2' because there was only 1 overload after this filtering above
for _, i := range s.constIdxs {
constExpr := exprs[i].(Constant)
s.overloadIdxs = filterOverloads(s.overloads, s.overloadIdxs,
func(o overloadImpl) bool {
semaCtx := MakeSemaContext()
_, err := constExpr.ResolveAsType(ctx, &semaCtx, o.params().GetAt(i))
return err == nil
})
}
This appears to be due to a check to convert the string into a date, but I am not sure why '1' is a valid date but '2' is not so I am not sure if there are other issues.
This was preventing further filtering from happening below for '2' which resulted in 1 overload instead of 0.
1 level up in func (expr *UnaryExpr) TypeCheck(...) the 0 overloads are correctly returning an error, but 1 overload is resulting in a result being returned.
if len(fns) != 1 {
var desStr string
if desired.Family() != types.AnyFamily {
desStr = fmt.Sprintf(" (desired <%s>)", desired)
}
sig := fmt.Sprintf("%s <%s>%s", expr.Operator, exprReturn, desStr)
if len(fns) == 0 {
return nil,
pgerror.Newf(pgcode.InvalidParameterValue, unsupportedUnaryOpErrFmt, sig)
}
fnsStr := formatCandidates(expr.Operator.String(), fns)
err = pgerror.Newf(pgcode.AmbiguousFunction, ambiguousUnaryOpErrFmt, sig)
err = errors.WithHintf(err, candidatesHintFmt, fnsStr)
return nil, err
}
Will this affect other code that calls typeCheckOverloadedExprs(...)? Should all short circuits be removed?
<!-- Sent from Reviewable.io -->
|
pkg/sql/sem/tree/overload.go, line 746 at r1 (raw file):
yeah. looks like the test failing here suggest that this check was necessary for certain checks.
that doesn't seem right. otherwise, i'd expect this to work: what does it think |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/overload.go, line 746 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Will this affect other code that calls typeCheckOverloadedExprs(...)?
yeah. looks like the test failing here suggest that this check was necessary for certain checks.
This appears to be due to a check to convert the string into a date,
that doesn't seem right. otherwise, i'd expect this to work:
[email protected]:26257/defaultdb> select '1'::date; ERROR: parsing as type date: missing required date fields SQLSTATE: 22007 DETAIL: Wanted: [ Year Day Era Hour Minute Second Nanos Meridian TZHour TZMinute TZSecond ] Already found in input: [ Month ] [email protected]:26257/defaultdb> select '2'::date; ERROR: parsing as type date: missing required date fields SQLSTATE: 22007 DETAIL: Wanted: [ Year Day Era Hour Minute Second Nanos Meridian TZHour TZMinute TZSecond ] Already found in input: [ Month ]what does it think
2is when we return early is, and why doesn't that result in an error message?
Looking at it further, it is actually not related to a date/time because ptCtx is being ignored in the taken branch inside ParseAndRequireString
ptCtx := simpleParseTimeContext{
// We can return any time, but not the zero value - it causes an error when
// parsing "yesterday".
RelativeParseTime: time.Date(2000, time.January, 2, 3, 4, 5, 0, time.UTC),
}
if semaCtx != nil {
ptCtx.DateStyle = semaCtx.DateStyle
ptCtx.IntervalStyle = semaCtx.IntervalStyle
}
val, dependsOnContext, err := ParseAndRequireString(typ, expr.s, ptCtx)
func ParseAndRequireString(
t *types.T, s string, ctx ParseTimeContext,
) (d Datum, dependsOnContext bool, err error) {
switch t.Family() {
case types.ArrayFamily:
d, dependsOnContext, err = ParseDArrayFromString(ctx, s, t.ArrayContents())
case types.BitFamily:
r, err := ParseDBitArray(s)
if err != nil {
return nil, false, err
}
d = formatBitArrayToType(r, t)
The difference still stands though that the reason '1' and '2' are different is because of an implicit conversion to bit array (1 can be converted which prevents a short circuit thereby allowing later filtering to 0) in cockroach that is not in postgres combined with the early return logic for having 1 overload.
|
pkg/sql/sem/tree/overload.go, line 746 at r1 (raw file): Previously, ecwall (Evan Wall) wrote…
another case of our type inference system not being great :'( |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/overload.go, line 746 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
another case of our type inference system not being great :'(
I'm now trying a new strategy: instead of preventing all strings from being implicitly cast, it now will display an ambiguous unary operator error (which includes a list of the ambiguities) instead of an unsupported unary operator error (which gave no additional info).
|
thanks for digging though all this! i like this approach |
different fixes #74493 Release note (sql change): Return ambiguous unary operator error for ambiguous input like ~'1' which can be interpreted as an integer (resulting in -2) or a bit string (resulting in 0). Release justification: Improves a confusing error message saying that an operator is invalid instead of ambiguous.
|
bors r=otan |
|
Build failed (retrying...): |
|
Build succeeded: |
fixes #74493
Release note (sql change): Return ambiguous unary operator error for ambiguous input
like ~'1' which can be interpreted as an integer (resulting in -2) or a bit string
(resulting in 0).
Release justification: Improves a confusing error message saying that an operator is
invalid instead of ambiguous.