[GitHub] [flink] wuchong commented on a change in pull request #16588: [FLINK-22936] Support column comment in Schema and ResolvedSchema

[GitBox <gi...@apache.org>]

wuchong commented on a change in pull request #16588:
URL: https://github.com/apache/flink/pull/16588#discussion_r676235188



##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/Schema.java
##########
@@ -221,6 +221,15 @@ public Builder fromColumns(List<UnresolvedColumn> unresolvedColumns) {
             return this;
         }
 
+        /** Apply comment to the previous column. */
+        public Builder withComment(@Nullable String comment) {
+            if (columns.size() > 0) {

Review comment:
       Please throw an `IllegalArgumentException` if there is no preceding column defined. 

##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/Column.java
##########
@@ -45,9 +45,18 @@
 
     protected final DataType dataType;
 
+    protected final @Nullable String comment;
+
     private Column(String name, DataType dataType) {
         this.name = name;
         this.dataType = dataType;
+        this.comment = null;
+    }
+
+    private Column(String name, DataType dataType, @Nullable String comment) {

Review comment:
       Ditto. Please share constructor implementation. 

##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/Schema.java
##########
@@ -665,13 +687,31 @@ public int hashCode() {
             this.dataType = dataType;

Review comment:
       We can share single constructor implementation by calling `this(columnName, dataType, null)`. 




-- 
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-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org