DRILL-6431: Unnest operator requires table and a single column alias #1277
Conversation
| * If not, insert a project renaming them. | ||
| */ | ||
| public RelNode getCorrelateInput(int offset, RelNode input) { | ||
| assert DrillJoinRelBase.uniqueFieldNames(input.getRowType()); |
|
|
||
| List<RelNode> reNamedChildren = Lists.newArrayList(); | ||
|
|
||
| RelNode left = prel.getCorrelateInput(0, children.get(0)); |
There was a problem hiding this comment.
Although the general logic for this is similar to that of Joins, didn't we want to make one distinction by only renaming the right side (unnest side) of Correlate ? In the current implementation both left and right would be eligible but will that ever be needed ?
There was a problem hiding this comment.
Yes. We wanted to make renaming only on right side. When I looked at the code it looks like all the time it should modify only the right side, because it first checks the left side and then the right side. Hence I thought same logic should work. Please let me know if I am missing anything here.
There was a problem hiding this comment.
Ok, you might want to add a TODO to indicate that this code could potentially be consolidated with JoinPrelRenameVisitor in the future.
| @@ -261,6 +263,11 @@ protected void validateFrom( | |||
| } | |||
| changeNamesIfTableIsTemporary(tempNode); | |||
| } | |||
| else if (((SqlCall) node).operand(0).getKind() == SqlKind.UNNEST) { | |||
| if (((SqlCall) node).operandCount() < 3) { | |||
| throw RESOURCE.validationError("Alias column name is required for UNNEST").ex(); | |||
There was a problem hiding this comment.
Alias table and column name ? Also, would be good to add 1 negative unit test that exercises this error.
| public void testMultipleBatchesLateral_WithLimitFilterInSubQuery() throws Exception { | ||
| String sql = "SELECT customer.c_name, customer.c_address, orders.o_orderkey, orders.o_totalprice " + | ||
| "FROM dfs.`lateraljoin/multipleFiles` customer, LATERAL " + | ||
| "(SELECT O.o.o_orderkey as o_orderkey, O.o.o_totalprice as o_totalprice FROM UNNEST(customer.c_orders) O(o) WHERE O.o.o_totalprice > 100000 LIMIT 2) " + |
There was a problem hiding this comment.
Minor nit: could you pls use different letter for the table name and column name in the alias (the upper and lower case 'o' is easy to get mixed up).
There was a problem hiding this comment.
Sure. will do it.
HanumathRao
changed the title
|
@amansinha100 @parthchandra Thank you for the review. |
| import static junit.framework.TestCase.fail; | ||
|
|
||
| public class TestE2EUnnestAndLateral extends BaseTestQuery { | ||
| //private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(TestE2EUnnestAndLateral.class); |
|
|
||
| List<RelNode> reNamedChildren = Lists.newArrayList(); | ||
|
|
||
| RelNode left = prel.getCorrelateInput(0, children.get(0)); |
There was a problem hiding this comment.
Ok, you might want to add a TODO to indicate that this code could potentially be consolidated with JoinPrelRenameVisitor in the future.
|
+1 (see couple of minor comments above). |
…to be specified. Fixing the issues related to star column renaming, same field name renaming and also enforcing that an alias column is required for the unnest operator.
|
@amansinha100 Thank you for the review. Done the changes. @sohami I have done the required refactoring of the code. Please do let me know if it is fine. |
|
+1 LGTM as well. |
@amansinha100 @parthchandra Please review this PR.
This PR includes following changes.
There are two commits in this PR. One commit (ie DRILL-6419) is about E2E integration test suite which was introduced by @sohami . There are some changes made to this test suite to provide an alias column for unnest operator.