Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12977 )
Change subject: IMPALA-5351: Support storing column comment of kudu table ...................................................................... Patch Set 2: (6 comments) Thanks for the patch! http://gerrit.cloudera.org:8080/#/c/12977/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12977/2//COMMIT_MSG@9 PS2, Line 9: This patch intends to support storing column comment of kudu table : on impala side. it's okay if we don't it in this patch, but do we plan to support COMMENT ON syntax for Kudu tables? http://gerrit.cloudera.org:8080/#/c/12977/2//COMMIT_MSG@12 PS2, Line 12: Belows tests passed: : 1) creata kudu table with column comment; : 2) alter kudu table with (add/alter[delete] column comment); : 3) show create kudu table; : 4) describe kudu table; Can you add tests into the following files? 1. AnalyzeDDLTest.java 2. test_ddl.py http://gerrit.cloudera.org:8080/#/c/12977/2/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java File fe/src/main/java/org/apache/impala/catalog/KuduColumn.java: http://gerrit.cloudera.org:8080/#/c/12977/2/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@88 PS2, Line 88: String comment = null; : if (!colSchema.getComment().isEmpty()) comment = colSchema.getComment(); nit: can be inlined String comment = (!colSchema.getComment().isEmpty()) ? (!colSchema.getComment().isEmpty()) : null; http://gerrit.cloudera.org:8080/#/c/12977/2/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java@116 PS2, Line 116: String comment = null; : if (column.isSetComment() && !column.getComment().isEmpty()) { : comment = column.getComment(); : } nit; same as above. It can be inlined. http://gerrit.cloudera.org:8080/#/c/12977/2/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12977/2/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@298 PS2, Line 298: String comment = null; : if (!colSchema.getComment().isEmpty()) { : comment = colSchema.getComment(); : } nit: inline this. http://gerrit.cloudera.org:8080/#/c/12977/2/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@303 PS2, Line 303: type.toSql().toLowerCase(), : comment)); nit Impala doesn't use this code style. Continued indentation is 4 spaces. -- To view, visit http://gerrit.cloudera.org:8080/12977 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c Gerrit-Change-Number: 12977 Gerrit-PatchSet: 2 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Thu, 11 Apr 2019 16:58:30 +0000 Gerrit-HasComments: Yes
