allowed zero argument in typeinferer #3137
Conversation
|
@coocood @hanfei1991 PTAL |
|
LGTM |
plan/typeinferer.go
Outdated
| tp = types.NewFieldType(mysql.TypeNewDecimal) | ||
| default: | ||
| tp = types.NewFieldType(mysql.TypeDouble) | ||
| if len(x.Args) > 0 { |
There was a problem hiding this comment.
if len(x.Args) == 0 {
tp = types.NewFieldType(mysql.TypeNull)
break
}
// Normal conditions
There was a problem hiding this comment.
I guess it can be put before the starting of switch statement, because every case statement do the check.
There was a problem hiding this comment.
@tiancaiamao
Many functions have zero arguments and the type is not null.
There was a problem hiding this comment.
@tiancaiamao That was my approach. If we simple skip switch if Args has 0 length, then for function curr_time(), test will not pass.
| @@ -122,6 +122,7 @@ func (ts *testTypeInferrerSuite) TestInferType(c *C) { | |||
| // Functions | |||
| {"version()", mysql.TypeVarString, charset.CharsetUTF8, 0}, | |||
| {"count(c_int)", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag}, | |||
| {"abs()", mysql.TypeNull, charset.CharsetBin, mysql.BinaryFlag}, | |||
There was a problem hiding this comment.
add test cases for ceil/ceiling/floor/ifnull/nullif/fromunixtime/round.
There was a problem hiding this comment.
This approach will cause a mixed feature in one PR. I have to modify parser.y in order to let zero arguments passed into executor stage. It is better to file another PR to add these test you mentioned.
There was a problem hiding this comment.
I think since you've modified the type inferer of these functions,
it better to test it in the same PR.
There was a problem hiding this comment.
@zhexuany
We can add tests, and comment it out, then comment back after the other PR gets merged.
There was a problem hiding this comment.
@coocood yep. I plan to do so. I will add these test tonight.
zhexuany
force-pushed
the
fixed_bug_in_typeinfer
branch
from
c5123e3 to
3292ceb
Compare
added commented test case. Will uncomment in another PR.
|
@XuHuaiyu PTAL. |
zhexuany
force-pushed
the
fixed_bug_in_typeinfer
branch
from
3292ceb to
4f732d8
Compare
plan/typeinferer.go
Outdated
| @@ -292,19 +292,30 @@ func (v *typeInferrer) getFsp(x *ast.FuncCallExpr) int { | |||
| return 0 | |||
| } | |||
|
|
|||
| // handleFuncCallExpr ... | |||
| // TODO: (zhexuany) this function contains too much redundant things. Mybe replace with a map like | |||
This PR is related with #3124. While working on replacing
ExpressionwithExpressionListOpt, I foundselect abs();raising a panic. This wired behavior is caused byx.Args[0].GetType()in https://github.com/pingcap/tidb/pull/3137/files#diff-686e374c1af6ac094dcaed1db4f0fd44R303. We need check the length ofx.Args. If it is 0, we need create a Null FieldType.