Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10754 )
Change subject: IMPALA-6918: Implement COMMENT ON COLUMN ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10754/6//COMMIT_MSG@12 PS6, Line 12: 'comment' State somewhere that this can be null and define the expected behavior for this case. http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3611 PS6, Line 3611: if (!catalog_.tryLockTable(tbl)) { : throw new InternalException(String.format("Error altering table/view %s due to " + : "lock contention.", tbl.getFullName())); : } this is the fourth time this error message and check is done... its bound to become inconsistent, pls factor it into a separate method. http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3636 PS6, Line 3636: updates nit: update http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/10754/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@4027 PS6, Line 4027: new Pair<>("fun what is this case adding to the one on L4025? http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl.py@337 PS6, Line 337: is null".fo since the null case follows '' (empty string), how do you know that null had an effect vs. had no effect? I think it would be more obvious if the null case was applied to a column with a non-empty comment. http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py File tests/metadata/test_ddl_base.py: http://gerrit.cloudera.org:8080/#/c/10754/6/tests/metadata/test_ddl_base.py@107 PS6, Line 107: print(result) for debugging? -- To view, visit http://gerrit.cloudera.org:8080/10754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8976705b27e98c1540941d0a6cba0e18bad48437 Gerrit-Change-Number: 10754 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 05 Jul 2018 06:20:51 +0000 Gerrit-HasComments: Yes
