twalthr commented on a change in pull request #17381:
URL: https://github.com/apache/flink/pull/17381#discussion_r718413128
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/DataTypes.java
##########
@@ -96,6 +97,15 @@
@PublicEvolving
public final class DataTypes {
+ /**
+ * Create {@link DataType} from a {@link LogicalType}.
+ *
+ * @return the {@link LogicalType} converted to a {@link DataType}.
+ */
+ public static DataType of(LogicalType logicalType) {
Review comment:
please split every API change into a separate commit
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/FieldsDataType.java
##########
@@ -57,24 +63,105 @@ public FieldsDataType(LogicalType logicalType,
List<DataType> fieldDataTypes) {
}
@Override
- public DataType notNull() {
+ public FieldsDataType notNull() {
Review comment:
this is API breaking, we should not do this. as we have seen recently on
the dev@ ML
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/utils/DataTypeUtils.java
##########
@@ -334,12 +334,15 @@ public static ResolvedSchema
expandCompositeTypeToSchema(DataType dataType) {
return Collections.singletonList(dataType);
}
- /** Returns the names of the flat representation in the first level of the
given data type. */
+ /**
+ * Returns the names of the flat representation of the given data type. In
case of {@link
+ * StructuredType}, returns the deep list of field names of the structure.
Review comment:
the original JavaDoc was more correct: `in the first level` we don't do
it deep in the sense of going through children as well
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/DataType.java
##########
@@ -86,10 +87,29 @@ public LogicalType getLogicalType() {
return conversionClass;
}
+ /**
+ * Returns the children of this data type, if any. Returns an empty list
if this data type is
+ * atomic.
+ *
+ * @return the children data types
+ */
public abstract List<DataType> getChildren();
+ /**
+ * Visit this data type.
+ *
+ * @return the result of the visit
+ */
public abstract <R> R accept(DataTypeVisitor<R> visitor);
+ /**
+ * Creates a {@link DataType} from this instance with internal data
structures conversion
+ * classes.
Review comment:
give an example e.g. `RowData` instead of `Row`, mention that it also
updated the nested fields
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/factories/DynamicTableFactory.java
##########
@@ -85,5 +88,23 @@
/** Whether the table is temporary. */
boolean isTemporary();
+
+ /**
+ * Shortcut for {@code
getCatalogTable().getResolvedSchema().toPhysicalRowDataType()}.
+ *
+ * @see ResolvedSchema#toPhysicalRowDataType()
+ */
+ default FieldsDataType getPhysicalRowDataType() {
+ return
getCatalogTable().getResolvedSchema().toPhysicalRowDataType();
+ }
+
+ /**
+ * Shortcut for {@code
getCatalogTable().getResolvedSchema().getPrimaryKeyFields()}.
+ *
+ * @see ResolvedSchema#getPrimaryKeyFields()
+ */
+ default List<String> getPrimaryKeyFields() {
Review comment:
as mentioned in the JIRA issue, a `int[]` would be more useful. in
general, we don't handle this nicely at the moment. actually according to SQL a
column must not have a unique name. positions might be more useful for
connector developers.
##########
File path:
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/DataType.java
##########
@@ -86,10 +87,29 @@ public LogicalType getLogicalType() {
return conversionClass;
}
+ /**
+ * Returns the children of this data type, if any. Returns an empty list
if this data type is
+ * atomic.
+ *
+ * @return the children data types
+ */
public abstract List<DataType> getChildren();
+ /**
+ * Visit this data type.
+ *
+ * @return the result of the visit
+ */
public abstract <R> R accept(DataTypeVisitor<R> visitor);
Review comment:
we should avoid meaningless JavaDocs that confuse more than they help.
--
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]
