Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java: http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1810 PS2, Line 1810: a nit: remove (same for below) http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1810 PS2, Line 1810: table nit: remove (same for below) http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1879 PS2, Line 1879: data what data? http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1890 PS2, Line 1890: style = TDescribeOutputStyle.MINIMAL; is there a reason for skipping EXTENDED for alltypessmall? http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1903 PS2, Line 1903: DescribeResult move to before use (L1809) http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1904 PS2, Line 1904: columns columns_ http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1905 PS2, Line 1905: location location_ also, what is this? an example of how describe output maps here would be useful. http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1944 PS2, Line 1944: if (sty use a switch instead. http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1946 PS2, Line 1946: getString_val should these be case insensitive? is trim no longer needed? http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1953 PS2, Line 1953: Location does this case matter? http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1956 PS2, Line 1956: r++; not obvious what is r doing... it looks like a row idx but gets bumped per column (L1960) -- To view, visit http://gerrit.cloudera.org:8080/10643 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f Gerrit-Change-Number: 10643 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 07 Jun 2018 23:17:27 +0000 Gerrit-HasComments: Yes
