You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/06/07 21:19:58 UTC
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10643
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
IMPALA-7144: Re-enable TestDescribeTableResults
This patch makes the TestDescribeTableResults more robust by only
comparing the information that the authorization cares about instead
of comparing all output in DESCRIBE. This change will avoid any unnecessary
changes to AuthorizationTest if HMS updates the DESCRIBE output.
The test is also updated to support standalone execution without relying
on other tests be executed first since it can cause the test to be flaky
especially if the tests in AuthorizationTest are executed in parallel.
Testing:
- Ran all FE tests
Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
1 file changed, 142 insertions(+), 182 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10643/2
--
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: newchange
Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 7: Code-Review+2
--
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: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 17:03:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
IMPALA-7144: Re-enable TestDescribeTableResults
This patch makes the TestDescribeTableResults more robust by only
comparing the information that the authorization cares about instead
of comparing all output in DESCRIBE. This change will avoid any unnecessary
changes to AuthorizationTest if HMS updates the DESCRIBE output.
The test is also updated to support standalone execution without relying
on other tests be executed first since it can cause the test to be flaky
especially if the tests in AuthorizationTest are executed in parallel.
Testing:
- Ran all FE tests
Cherry-picks: not for 2.x
Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
1 file changed, 174 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10643/6
--
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: newpatchset
Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10643/4/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/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1974
PS4, Line 1974: Precondi
> does it matter if location is set multiple times for a given set of rows?
There can only be one location. I'll add a precondition.
http://gerrit.cloudera.org:8080/#/c/10643/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1979
PS4, Line 1979: pty li
> that's not the index that is accessed (++rowIdx).
Ah yeah. Done.
--
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: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 13:50:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10643/5/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/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1979
PS5, Line 1979: rowIdx += 2; // Skips over the first empty line.
: Preconditions.checkElementIndex(rowIdx, rows.size());
> simplify:
Done
http://gerrit.cloudera.org:8080/#/c/10643/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1985
PS5, Line 1985: rowIdx++;
> simplify:
Done
--
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: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 16:04:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 3:
(5 comments)
several small suggestions to make things clearer, then lgtm.
http://gerrit.cloudera.org:8080/#/c/10643/3/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/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1811
PS3, Line 1811: columns_
add a comment to clarify if these are column names or values.
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1812
PS3, Line 1812: location_;
add a comment, e.g., "the value of the describe result's location field"
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1950
PS3, Line 1950:
cleanup ws
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1961
PS3, Line 1961: [#col_name,data_type
example makes it clearer. too not distract this method, pls minimize this a bit.. for example, its not needed to have so many column example (two #col_name makes sense to illustrate the repetition). the location field should stay. a few fields that are not needed is also fine, but would be good to remove the majority of these.
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2024
PS3, Line 2024: rowIdx
moving this offset in a loop without a check is error prone. pls add a precondition before each rows access that rowIdx < rows.size()
--
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: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 05:10:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10643/4/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/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1974
PS4, Line 1974: location
does it matter if location is set multiple times for a given set of rows?
http://gerrit.cloudera.org:8080/#/c/10643/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1979
PS4, Line 1979: rowIdx
that's not the index that is accessed (++rowIdx).
--
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: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 06:54:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10643/5/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/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1979
PS5, Line 1979: rowIdx++; // Ignore the first empty line.
: Preconditions.checkElementIndex(rowIdx + 1, rows.size());
simplify:
rowIdx += 2; // Skips over the first empty line
Preconditions.checkElementIndex(rowIdx, rows.size());
cols = rows.get(rowIdx).getColVals();
http://gerrit.cloudera.org:8080/#/c/10643/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1985
PS5, Line 1985: Preconditions.checkElementIndex(rowIdx + 1, rows.size());
simplify:
++rowIdx;
Preconditions.checkElementIndex(rowIdx, rows.size());
cols = rows.get(rowIdx).getColVals();
--
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: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 15:59:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 7: Verified+1
--
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: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 20:29:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
IMPALA-7144: Re-enable TestDescribeTableResults
This patch makes the TestDescribeTableResults more robust by only
comparing the information that the authorization cares about instead
of comparing all output in DESCRIBE. This change will avoid any unnecessary
changes to AuthorizationTest if HMS updates the DESCRIBE output.
The test is also updated to support standalone execution without relying
on other tests be executed first since it can cause the test to be flaky
especially if the tests in AuthorizationTest are executed in parallel.
Testing:
- Ran all FE tests
Cherry-picks: not for 2.x
Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
1 file changed, 173 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10643/5
--
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: newpatchset
Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
IMPALA-7144: Re-enable TestDescribeTableResults
This patch makes the TestDescribeTableResults more robust by only
comparing the information that the authorization cares about instead
of comparing all output in DESCRIBE. This change will avoid any unnecessary
changes to AuthorizationTest if HMS updates the DESCRIBE output.
The test is also updated to support standalone execution without relying
on other tests be executed first since it can cause the test to be flaky
especially if the tests in AuthorizationTest are executed in parallel.
Testing:
- Ran all FE tests
Cherry-picks: not for 2.x
Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Reviewed-on: http://gerrit.cloudera.org:8080/10643
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
1 file changed, 174 insertions(+), 186 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
IMPALA-7144: Re-enable TestDescribeTableResults
This patch makes the TestDescribeTableResults more robust by only
comparing the information that the authorization cares about instead
of comparing all output in DESCRIBE. This change will avoid any unnecessary
changes to AuthorizationTest if HMS updates the DESCRIBE output.
The test is also updated to support standalone execution without relying
on other tests be executed first since it can cause the test to be flaky
especially if the tests in AuthorizationTest are executed in parallel.
Testing:
- Ran all FE tests
Cherry-picks: not for 2.x
Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
1 file changed, 211 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10643/3
--
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: newpatchset
Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Jun 2018 23:17:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
IMPALA-7144: Re-enable TestDescribeTableResults
This patch makes the TestDescribeTableResults more robust by only
comparing the information that the authorization cares about instead
of comparing all output in DESCRIBE. This change will avoid any unnecessary
changes to AuthorizationTest if HMS updates the DESCRIBE output.
The test is also updated to support standalone execution without relying
on other tests be executed first since it can cause the test to be flaky
especially if the tests in AuthorizationTest are executed in parallel.
Testing:
- Ran all FE tests
Cherry-picks: not for 2.x
Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
1 file changed, 172 insertions(+), 186 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10643/4
--
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: newpatchset
Gerrit-Change-Id: I3aeaecf5b6d906a66d338e165a6d506e3964563f
Gerrit-Change-Number: 10643
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/10643/3/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/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1811
PS3, Line 1811:
> add a comment to clarify if these are column names or values.
Done
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1812
PS3, Line 1812: ring> colu
> add a comment, e.g., "the value of the describe result's location field"
Done
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1950
PS3, Line 1950:
> cleanup ws
Done
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1961
PS3, Line 1961: [month,int,],
> example makes it clearer. too not distract this method, pls minimize this a
Done
http://gerrit.cloudera.org:8080/#/c/10643/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@2024
PS3, Line 2024: privi
> moving this offset in a loop without a check is error prone. pls add a prec
Done
--
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: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 05:39:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 3:
(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:
> nit: remove (same for below)
Done
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1810
PS2, Line 1810:
> nit: remove (same for below)
Done
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1879
PS2, Line 1879:
> what data?
It should be metadata.
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1890
PS2, Line 1890: "hdfs://localhost:20500/test-warehouse/alltypesagg"
> is there a reason for skipping EXTENDED for alltypessmall?
Nope. I'll add.
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1903
PS2, Line 1903: atements force
> move to before use (L1809)
Done
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1904
PS2, Line 1904: indepe
> columns_
Done
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1905
PS2, Line 1905: ctional.
> location_
Done
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1944
PS2, Line 1944: //
> use a switch instead.
Done
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1946
PS2, Line 1946:
> should these be case insensitive?
The column names in DESCRIBE output are always in lower case. I'll trim the output.
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1953
PS2, Line 1953:
> does this case matter?
We want to match the same exact thing. So it's better to match the output in DESCRIBE.
http://gerrit.cloudera.org:8080/#/c/10643/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java@1956
PS2, Line 1956: eak;
> not obvious what is r doing... it looks like a row idx but gets bumped per
I'll rename r to rowIdx and add more comments.
--
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: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 01:57:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 6: Code-Review+2
--
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: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 16:06:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7144: Re-enable TestDescribeTableResults
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10643 )
Change subject: IMPALA-7144: Re-enable TestDescribeTableResults
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2638/
--
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: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 17:03:48 +0000
Gerrit-HasComments: No