fsk119 commented on code in PR #26553:
URL: https://github.com/apache/flink/pull/26553#discussion_r2097214116
##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -2573,7 +2575,7 @@ private SqlNode registerFrom(
if (operand instanceof SqlBasicCall) {
final SqlBasicCall call1 = (SqlBasicCall) operand;
final SqlOperator op = call1.getOperator();
- if (op instanceof SqlWindowTableFunction
+ if ((op instanceof SqlWindowTableFunction || op instanceof
SqlMLTableFunction)
Review Comment:
I am confuesd about the change and I can not produce a case have the same
error as SqlWindowFunction. I am not sure FLINK-35619 is a good solution here,
which tries to validate a node when validating namespace. However, in most
cases, validator should register the namespace for the node and then validate.
```
private SqlNode validateScopedExpression(SqlNode topNode,
SqlValidatorScope scope) {
SqlNode outermostNode = performUnconditionalRewrites(topNode, false);
cursorSet.add(outermostNode);
top = outermostNode;
TRACER.trace("After unconditional rewrite: {}", outermostNode);
if (outermostNode.isA(SqlKind.TOP_LEVEL)) {
// 1. REGISTER namespace
registerQuery(scope, null, outermostNode, outermostNode, null,
false);
}
// 2. VALIDATE node
outermostNode.validate(this, scope);
if (!outermostNode.isA(SqlKind.TOP_LEVEL)) {
// force type derivation so that we can provide it to the
// caller later without needing the scope
deriveType(scope, outermostNode);
}
TRACER.trace("After validation: {}", outermostNode);
return outermostNode;
}
```
How about removing the change here?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java:
##########
@@ -371,7 +374,14 @@ protected void addToSelectList(
final SqlBasicCall call = (SqlBasicCall) node;
final SqlOperator operator = call.getOperator();
- if (operator instanceof SqlWindowTableFunction) {
+ if (node instanceof SqlExplicitModelCall) {
+ // Convert it so that model can be accessed in planner.
SqlExplicitModelCall
+ // from parser can't access model.
+ SqlExplicitModelCall modelCall = (SqlExplicitModelCall) node;
Review Comment:
How about we only rewrite the call if the model is not null here. For
example, `SqlValidatorImpl#performUnconditionalRewrites` override the operator
if the function is found.
```
if (call.getOperator() instanceof SqlUnresolvedFunction) {
assert call instanceof SqlBasicCall;
final SqlUnresolvedFunction function = (SqlUnresolvedFunction)
call.getOperator();
// This function hasn't been resolved yet. Perform
// a half-hearted resolution now in case it's a
// builtin function requiring special casing. If it's
// not, we'll handle it later during overload resolution.
final List<SqlOperator> overloads = new ArrayList<>();
opTab.lookupOperatorOverloads(
function.getNameAsId(),
function.getFunctionType(),
SqlSyntax.FUNCTION,
overloads,
catalogReader.nameMatcher());
if (overloads.size() == 1) {
((SqlBasicCall) call).setOperator(overloads.get(0));
}
}
```
I think we can have the same logic here
```
if (node instanceof SqlExplicitModelCall) {
// Convert it so that model can be accessed in planner.
SqlExplicitModelCall
// from parser can't access model.
FlinkCalciteCatalogReader catalogReader =
(FlinkCalciteCatalogReader) getCatalogReader();
CatalogSchemaModel model =
catalogReader.getModel(((SqlExplicitModelCall) node).getIdentifier());
if (model != null) {
return new SqlModelCall(model);
}
}
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java:
##########
@@ -371,7 +374,14 @@ protected void addToSelectList(
final SqlBasicCall call = (SqlBasicCall) node;
final SqlOperator operator = call.getOperator();
- if (operator instanceof SqlWindowTableFunction) {
+ if (node instanceof SqlExplicitModelCall) {
+ // Convert it so that model can be accessed in planner.
SqlExplicitModelCall
+ // from parser can't access model.
+ SqlExplicitModelCall modelCall = (SqlExplicitModelCall) node;
+ return new SqlModelCall(modelCall);
+ }
+
+ if (operator instanceof SqlWindowTableFunction || operator instanceof
SqlMLTableFunction) {
Review Comment:
Can we add a case to verify the change here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
