luoyuxia commented on code in PR #22213:
URL: https://github.com/apache/flink/pull/22213#discussion_r1141587381
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverter.java:
##########
@@ -37,6 +47,19 @@
*/
Operation convertSqlNode(S node, ConvertContext context);
+ /**
+ * Returns the {@link SqlKind SqlKinds} of {@link SqlNode SqlNodes} that
the {@link
+ * SqlNodeConverter} supports to convert.
+ *
+ * <p>If a {@link SqlNodeConverter} return s a non-empty SqlKinds, the
conversion framework
Review Comment:
nit:
```suggestion
* <p>If a {@link SqlNodeConverter} returns a non-empty SqlKinds, the
conversion framework
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverter.java:
##########
@@ -37,6 +47,19 @@
*/
Operation convertSqlNode(S node, ConvertContext context);
+ /**
+ * Returns the {@link SqlKind SqlKinds} of {@link SqlNode SqlNodes} that
the {@link
+ * SqlNodeConverter} supports to convert.
+ *
+ * <p>If a {@link SqlNodeConverter} return s a non-empty SqlKinds, the
conversion framework
+ * prefer to match SqlKind of SqlNode instead of matching class of SqlNode.
Review Comment:
nit:
```suggestion
* prefers to match SqlKind of SqlNode instead of matching class of
SqlNode.
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverter.java:
##########
@@ -37,6 +47,19 @@
*/
Operation convertSqlNode(S node, ConvertContext context);
+ /**
+ * Returns the {@link SqlKind SqlKinds} of {@link SqlNode SqlNodes} that
the {@link
+ * SqlNodeConverter} supports to convert.
+ *
+ * <p>If a {@link SqlNodeConverter} return s a non-empty SqlKinds, the
conversion framework
+ * prefer to match SqlKind of SqlNode instead of matching class of SqlNode.
Review Comment:
By saying **prefer**, for a instance of `SqlNodeConverter`, I would like to
expect it will first try to use `SqlKind` to match, if can't match, it'll then
fall back to use class of SqlNode to match.
But from implemetation, seems it'll **only** use `SqlKind` to match.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverters.java:
##########
@@ -46,19 +49,51 @@ public class SqlNodeConverters {
@SuppressWarnings({"unchecked", "rawtypes"})
public static Optional<Operation> convertSqlNode(
SqlNode validatedSqlNode, ConvertContext context) {
- SqlNodeConverter converter =
CONVERTERS.get(validatedSqlNode.getClass());
- if (converter != null) {
- return Optional.of(converter.convertSqlNode(validatedSqlNode,
context));
+ // match by class first
+ SqlNodeConverter classConverter =
CLASS_CONVERTERS.get(validatedSqlNode.getClass());
Review Comment:
Have a question in here.
For the case that if we have two implemtation of `SqlNodeConverter`.
1: `SqlInsertConverter` which returns `Optional.of(SqlKind.INSERT)` in
method `supportedSqlKinds`
2: `SqlRichInsertConverter`which implements
`SqlNodeConverter<RichSqlInsert>` but haven't overriden the method
`supportedSqlKinds` .
If both `SqlInsertConverter` and `SqlRichInsertConverter` are registered,
for `RichSqlInsert`,
in here, it will then use `SqlRichInsertConverter` to do conversion as it
match by class first.
Is it expected?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConverter.java:
##########
@@ -23,10 +23,20 @@
import org.apache.flink.table.planner.utils.Expander;
import org.apache.calcite.rel.RelRoot;
+import org.apache.calcite.sql.SqlKind;
import org.apache.calcite.sql.SqlNode;
import org.apache.calcite.sql.validate.SqlValidator;
-/** A converter to convert {@link SqlNode} instance into {@link Operation}. */
+import java.util.EnumSet;
+import java.util.Optional;
+
+/**
+ * A converter to convert {@link SqlNode} instance into {@link Operation}.
+ *
+ * <p>By default, a {@link SqlNodeConverter} only matches a specific SqlNode
class to convert which
+ * is defined by the parameter type {@code S}. But a {@link SqlNodeConverter}
can also matches a set
Review Comment:
nit
```suggestion
* is defined by the parameter type {@code S}. But a {@link
SqlNodeConverter} can also match a set
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
