Alex Behm has posted comments on this change. Change subject: IMPALA-3938: Disallow implicit references between nested collections. ......................................................................
Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/4079/4/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java: Line 583: final String t8Tbl = "d.t8"; While this change does follow the main spirit of my suggestion, I think the abstractions are pretty heavy weight. The intention was to avoid copy+pasting several variants of a test case, and to reduce the test code. The test code should be readable and minimal in the sense that it only introduces minimal new abstractions and "boilerplate" overhead. For example, to attack this specific task at hand I would have written the following helper function: private void testImplicitPathFailure(String parent, boolean parentIsCollection, String collection, String... fields) { String[] parentElements = parent.split("\\."); String implicitAlias = parentElements[parentElements.length - 1]; String explicitAlias = "x"; for (String field: fields) { // Tests that the path in the select list item does not resolve. AnalysisError(String.format("select %s from %s.%s", field, parent, collection), String.format("Could not resolve column/field reference: '%s'", field)); AnalysisError(String.format("select %s.%s from %s.%s %s", explicitAlias, field, parent, collection, explicitAlias), String.format("Could not resolve column/field reference: '%s.%s'", explicitAlias, field)); if (parentIsCollection) { AnalysisError(String.format("select %s from %s join %s.%s", field, parent, implicitAlias, collection), String.format("Could not resolve column/field reference: '%s'", field)); AnalysisError(String.format("select %s.%s from %s %s join %s.%s", explicitAlias, field, parent, explicitAlias, explicitAlias, collection), String.format("Could not resolve column/field reference: '%s.%s'", explicitAlias, field)); } // Tests that the path in the last table reference does not resolve. AnalysisError(String.format("select 1 from %s.%s join %s.%s", parent, collection, collection, field), String.format("Could not resolve table reference: '%s.%s'", collection, field)); AnalysisError(String.format("select 1 from %s.%s %s join %s.%s", parent, collection, explicitAlias, explicitAlias, field), String.format("Could not resolve table reference: '%s.%s'", explicitAlias, field)); } } Then the tests can be written as: addTestTable("create table d.t8 (" + "c1 array<map<string, string>>, " + "c2 map<string,array<struct<a:int>>>, " + "c3 struct<s1:struct<a:array<array<struct<e:int,f:string>>>>>)"); testImplicitPathFailure("d.t8", true, "c1", "key", "value"); testImplicitPathFailure("d.t8", true, "c2", "pos"); testImplicitPathFailure("d.t8.c3.s1", false, "a", "f"); This also looks very similar to some of the existing tests we have that follow this pattern. -- To view, visit http://gerrit.cloudera.org:8080/4079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I703c7e9c8c9c95c9fee714767367373eec10cd0c Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Christopher Channing <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Christopher Channing <[email protected]> Gerrit-HasComments: Yes
