[FLINK-34312][table] Improve the handling of default node types for named parameters #24235
Conversation
There was a problem hiding this comment.
Thanks for your update. I left some minor suggestions. cc @xuyangzhong
...main/java/org/apache/flink/table/planner/functions/inference/OperatorBindingCallContext.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| return false; | ||
| return !argumentTypes.isEmpty(); |
There was a problem hiding this comment.
If the arugment types is empty, it means the function's parameter is fixed comparing to var-length parameters. At this point, I think it's better to align the concepts with calcite. What about isFixedParameters here and rename argumentTypes to fixedArgumentTypes?
hackergin
force-pushed
the
optional-update
branch
from
b49f248
to
5aaedcc
Compare
| return fixArgumentTypes.get(ordinal); | ||
| } | ||
|
|
||
| final RelDataType type = SqlTypeUtil.deriveType(this, operand); |
There was a problem hiding this comment.
@fsk119 Here I made some modifications. Previously it was: If it is fixArgumentType, then return argumentType. In fact, there is a problem. We should only return argumentType if it is a DEFAULT node. Other nodes should still return the actual operand's type.
| * <p>Lines 3739 ~ 3743, 6333 ~ 6339, Flink improves validating the SqlCall that uses named | ||
| * parameters, rearrange the order of sub-operands, and fill in missing operands with the default |
There was a problem hiding this comment.
can you please create a corresponding issue for Calcite to make it supported there as well?
There was a problem hiding this comment.
Thank you for the suggestion. I have created the corresponding JIRA ticket. You can find it at https://issues.apache.org/jira/browse/CALCITE-6245.
fsk119
force-pushed
the
optional-update
branch
from
5aaedcc
to
51cacbd
Compare
…amed parameters (apache#24235) Co-authored-by: Shengkai <1059623455@qq.com>
What is the purpose of the change
Improve the default node handing for named parameters.
Before SqlToRel, we can construct a DEFAULT operator with a return type that matches the argument type. This way, during the SqlToRel phase, there is no need for special handling of the DEFAULT node's type.
Brief change log
FlinkSqlCallBindingfor handing rewrite the type ofDEFAULToperatorVerifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation