Unnest array of row to multiple columns #10883

rongrong · 2018-06-21T04:34:59Z

This partially resolves #8151 (for array type).

wenleix

In general looks good to me. But my knowledge about analyzer and planner is very limited.

@martint , @kokosing would you mind also taking look? Thank you !

wenleix · 2018-06-21T04:57:46Z

presto-main/src/main/java/com/facebook/presto/operator/ArrayRowUnnester.java

+    private int position;
+    private int positionCount;
+
+    public ArrayRowUnnester(Type elementType)


What about just make constructor to take List<Type> fieldTypes ?

I thought about that. But setBlock expects a RowBlock, so it would be really strange for the constructor to take an arbitrary list of types. There are pros and cons for both. I think keeping the constructor consistent with other unnesters and make it obvious that this should be only used for RowType is better.

wenleix · 2018-06-21T05:00:25Z

presto-main/src/main/java/com/facebook/presto/operator/ArrayRowUnnester.java

+    implements Unnester
+{
+    private final List<Type> fieldTypes;
+    private ColumnarRow block;


What about just call it columnarRow?

Sure, both makes sense. I don't really care.

wenleix · 2018-06-21T05:09:08Z

presto-main/src/main/java/com/facebook/presto/operator/UnnestOperator.java

@@ -112,7 +113,12 @@ public UnnestOperator(OperatorContext operatorContext, List<Integer> replicateCh
        this.unnesters = new ArrayList<>(unnestTypes.size());
        for (Type type : unnestTypes) {
            if (type instanceof ArrayType) {
-                unnesters.add(new ArrayUnnester(((ArrayType) type).getElementType()));
+                if (((ArrayType) type).getElementType() instanceof RowType) {


Might consider put a local variable elementType = ((ArrayType) type).getElementType() and being reused ?

wenleix · 2018-06-21T05:34:20Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -575,6 +575,11 @@ public void testUnnest()
        assertQuery("SELECT b FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5]) t(a, b)", "SELECT * FROM VALUES 4, 5, NULL");
        assertQuery("SELECT count(*) FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5])", "SELECT 3");
        assertQuery("SELECT a FROM UNNEST(ARRAY['kittens', 'puppies']) t(a)", "SELECT * FROM VALUES ('kittens'), ('puppies')");
+        assertQuery("SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])", "SELECT * FROM VALUES (1, 2, 'a', true), (3, 4, 'b', false)");


nit: Add comment explaining these three tests.

No other test cases have comments. These test cases should be at least as self-explanatory for what they are testing as the others in the test. Do we have any criteria on when to add comment vs not?

wenleix · 2018-06-21T05:34:41Z

presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java

@@ -650,7 +661,17 @@ protected RelationPlan visitUnnest(Unnest node, Void context)
            Symbol inputSymbol = symbolAllocator.newSymbol(rewritten, type);
            argumentSymbols.add(inputSymbol);
            if (type instanceof ArrayType) {
-                unnestSymbols.put(inputSymbol, ImmutableList.of(unnestedSymbolsIterator.next()));
+                Type elementType = ((ArrayType) type).getElementType();


line 663 - 680 is same as line 562 - 580. And they are both doing unnest.

Would it make sense to refactor them? @martint @kokosing

I don't quite like how unnest is done through out the code base. the

if (type instanceof ArrayType) ... else if (type instanceof MapType) ...

is everywhere. If I were to do a refactoring, I'd prefer something doesn't require that. But I didn't see an easy way to do so. Would be more than happy to take suggestions.

Please extract the outer if as a static method.

I'm not sure what part of the following snippet you don't like

if (type instanceof ArrayType) ... else if (type instanceof MapType) ...

You could have done switch(type.getTypeSignature().getBase(). But that makes little difference.

kokosing · 2018-06-21T05:35:24Z

presto-main/src/main/java/com/facebook/presto/operator/ArrayRowUnnester.java

@@ -0,0 +1,63 @@
+package com.facebook.presto.operator;


kokosing · 2018-06-21T05:41:26Z

presto-main/src/main/java/com/facebook/presto/operator/ArrayRowUnnester.java

+import static com.google.common.base.Preconditions.checkState;
+import static java.util.Objects.requireNonNull;
+
+public class ArrayRowUnnester


ArrayOfRowsUnnester?

kokosing · 2018-06-21T05:51:04Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -575,6 +575,11 @@ public void testUnnest()
        assertQuery("SELECT b FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5]) t(a, b)", "SELECT * FROM VALUES 4, 5, NULL");
        assertQuery("SELECT count(*) FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5])", "SELECT 3");
        assertQuery("SELECT a FROM UNNEST(ARRAY['kittens', 'puppies']) t(a)", "SELECT * FROM VALUES ('kittens'), ('puppies')");
+        assertQuery("SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])", "SELECT * FROM VALUES (1, 2, 'a', true), (3, 4, 'b', false)");


can you please format this as:

assertQuery( actualQuery, expectedQuery); ```?

kokosing · 2018-06-21T05:52:12Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -575,6 +575,11 @@ public void testUnnest()
        assertQuery("SELECT b FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5]) t(a, b)", "SELECT * FROM VALUES 4, 5, NULL");
        assertQuery("SELECT count(*) FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5])", "SELECT 3");
        assertQuery("SELECT a FROM UNNEST(ARRAY['kittens', 'puppies']) t(a)", "SELECT * FROM VALUES ('kittens'), ('puppies')");
+        assertQuery("SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])", "SELECT * FROM VALUES (1, 2, 'a', true), (3, 4, 'b', false)");
+        assertQuery("SELECT x, y FROM (VALUES (ARRAY[ROW(1, 2), ROW(3, 4)])) AS t(a) CROSS JOIN UNNEST(a) t(x, y)", "SELECT * FROM VALUES (1, 2), (3, 4)");


why the output is not:

SELECT * FROM VALUES (1, 2, 3, 4)

There is only one array so I was expecting only one output row.

It's one array of two rows. Each array element will become one row in the final result right? So the first element ROW(1, 2) becomes (1, 2), and second element ROW(3, 4) becomes (3, 4).

What will be produced for:

SELECT x, y FROM (VALUES (ARRAY[ROW(1, 2), ROW('varchar', false])) AS t(a) CROSS JOIN UNNEST(a) t(x, y)

?

Also why the test

assertQuery("SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])", "SELECT * FROM VALUES (1, 2, 'a', true), (3, 4, 'b', false)");

does not produce 4 rows (one per each row in arrays)?

If I understand the semantic correctly, when you unnest an array, each element in the array will become 1 row. If you unnest two arrays, first element of both arrays will become 1 row, second element of both arrays will become the second row, etc. Things would be a little more obvious with "WITH ORDINALITY". Previously, we'd always unnest an array into one column, so if it's array, we'd unnest that to one column of type ROW. This PR would change it so it unnests the array to multiple columns defined by the fields in the row.

In your example, ARRAY[ROW(1, 2), ROW('varchar', false]) is not a valid array.

kokosing · 2018-06-21T05:54:44Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -575,6 +575,11 @@ public void testUnnest()
        assertQuery("SELECT b FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5]) t(a, b)", "SELECT * FROM VALUES 4, 5, NULL");
        assertQuery("SELECT count(*) FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5])", "SELECT 3");
        assertQuery("SELECT a FROM UNNEST(ARRAY['kittens', 'puppies']) t(a)", "SELECT * FROM VALUES ('kittens'), ('puppies')");
+        assertQuery("SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])", "SELECT * FROM VALUES (1, 2, 'a', true), (3, 4, 'b', false)");
+        assertQuery("SELECT x, y FROM (VALUES (ARRAY[ROW(1, 2), ROW(3, 4)])) AS t(a) CROSS JOIN UNNEST(a) t(x, y)", "SELECT * FROM VALUES (1, 2), (3, 4)");


Please also below queries:

SELECT x, y FROM (VALUES (ARRAY[ROW(1, 2), ROW('varchar', false)])) AS t(a) CROSS JOIN UNNEST(a) t(x, y) SELECT x, y FROM (VALUES (ARRAY[ROW(1)], ARRAY[ROW(2, 3)]) AS t(a) CROSS JOIN UNNEST(a) t(x, y) SELECT x, y FROM (VALUES (ARRAY[ROW(1)], ARRAY[ROW('varchar')]) AS t(a) CROSS JOIN UNNEST(a) t(x, y)

These queries not valid, so they won't test unnest array of rows.

Yes, but they should fail in some reasonable way. IMO this should be tested, but maybe not here.

findepi · 2018-06-21T08:16:49Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -575,6 +575,15 @@ public void testUnnest()
        assertQuery("SELECT b FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5]) t(a, b)", "SELECT * FROM VALUES 4, 5, NULL");
        assertQuery("SELECT count(*) FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5])", "SELECT 3");
        assertQuery("SELECT a FROM UNNEST(ARRAY['kittens', 'puppies']) t(a)", "SELECT * FROM VALUES ('kittens'), ('puppies')");
+        assertQuery(
+                "SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])",


in case of

SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)])

what are the column names?

They shouldn’t have one, just like every other place in the engine that produces anonymous rows or columns.

nezihyigitbasi · 2018-06-21T18:06:49Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

-                    outputFields.add(Field.newUnqualified(Optional.empty(), ((ArrayType) expressionType).getElementType()));
+                    Type elementType = ((ArrayType) expressionType).getElementType();
+                    if (elementType instanceof RowType) {
+                        outputFields.addAll(


Extracting a variable will make this more readable:

ImmutableList<Field> fields = elementType.getTypeParameters().stream() .map(type -> Field.newUnqualified(Optional.empty(), type)) .collect(toImmutableList()); outputFields.addAll(fields);

I agree. But I thought we prefer inlining variables if they are only used once? Happy to go either way. But can we have a consistent standard? (I can take "when it affects readability, use a variable, otherwise inline" as a standard, though that would leave some room for debates about what's "readable". So it's probably better to provide a clearer guideline.

haozhun

See inline comments below.

In addition, please add tests to TestUnnestOperator.

haozhun · 2018-06-26T22:33:05Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java

@@ -575,6 +575,15 @@ public void testUnnest()
        assertQuery("SELECT b FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5]) t(a, b)", "SELECT * FROM VALUES 4, 5, NULL");
        assertQuery("SELECT count(*) FROM UNNEST(ARRAY[1, 2, 3], ARRAY[4, 5])", "SELECT 3");
        assertQuery("SELECT a FROM UNNEST(ARRAY['kittens', 'puppies']) t(a)", "SELECT * FROM VALUES ('kittens'), ('puppies')");
+        assertQuery(
+                "SELECT * FROM UNNEST(ARRAY[ROW(1, 2), ROW(3, 4)], ARRAY[ROW('a', true), ROW('b', false)])",


Please cover double and structural type in this test case

haozhun · 2018-06-26T22:34:50Z

presto-main/src/main/java/com/facebook/presto/sql/planner/RelationPlanner.java

@@ -650,7 +661,17 @@ protected RelationPlan visitUnnest(Unnest node, Void context)
            Symbol inputSymbol = symbolAllocator.newSymbol(rewritten, type);
            argumentSymbols.add(inputSymbol);
            if (type instanceof ArrayType) {
-                unnestSymbols.put(inputSymbol, ImmutableList.of(unnestedSymbolsIterator.next()));
+                Type elementType = ((ArrayType) type).getElementType();


Please extract the outer if as a static method.

I'm not sure what part of the following snippet you don't like

if (type instanceof ArrayType) ... else if (type instanceof MapType) ...

You could have done switch(type.getTypeSignature().getBase(). But that makes little difference.

haozhun · 2018-06-26T22:45:53Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

+                        outputFields.addAll(
+                                elementType.getTypeParameters().stream()
+                                        .map(type -> Field.newUnqualified(Optional.empty(), type))
+                                        .collect(toImmutableList()));


I personally prefer using a for-loop here. If you prefer stream, you could do .forEach(outputFields::add). This way, you wouldn't need outputFields.addAll any more. You can change your code either way.

haozhun · 2018-06-26T22:49:06Z

presto-main/src/main/java/com/facebook/presto/operator/ArrayOfRowsUnnester.java

+    {
+        this.columnarRow = ColumnarRow.toColumnarRow(block);
+        this.position = 0;
+        this.positionCount = block == null ? 0 : block.getPositionCount();


NPE at line 72. ColumnarRow.toColumnarRow throws for null argument

martint

Some minor comments. Please look at test failures, too.

martint · 2018-07-18T15:13:57Z

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

+                        true),
+                booleanSessionProperty(
+                        LEGACY_UNNEST,
+                        "Using legacy unnest semantic, where unnest(array<row>) will create one column of type row",


array(row) is the canonical form to refer to a parametric type.

martint · 2018-07-18T15:15:36Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java

@@ -115,6 +115,7 @@
    private DataSize filterAndProjectMinOutputPageSize = new DataSize(500, KILOBYTE);
    private int filterAndProjectMinOutputPageRowCount = 256;
    private int maxGroupingSets = 2048;
+    private boolean legacyUnnestArrayRows = true;


Make this false by default. The old (broken) behavior should be available but not how it works out of the box.

martint · 2018-07-18T15:22:09Z

presto-tests/src/test/java/com/facebook/presto/tests/TestLegacyUnnestArrayRows.java

+@Test(singleThreaded = true)
+public class TestLegacyUnnestArrayRows
+{
+    private H2QueryRunner h2QueryRunner;


No need for H2 here. Take a look at how com.facebook.presto.sql.query.TestXXX work.

martint · 2018-07-18T15:26:48Z

presto-tests/src/test/java/com/facebook/presto/tests/TestLegacyUnnestArrayRows.java

+    }
+
+    @Test
+    public void testUnnestArrayRowsLegacyBehavorDisabled()


Move this test to a TestUnnest class in com.facebook.presto.sql.query, similar to TestGrouping, TestJoinUsing, etc. and leave TestLegacyUnnest in a separate class (like TestLegacyJoinUsing). It's easier to follow the code, keep concerns separated and when it's time to remove the feature, it's just a matter of deleting the class.

rongrong · 2018-07-23T23:00:40Z

@martint I addressed the comments. Mind take another look? Thanks!

martint

Minor comments, but otherwise looks good.

martint · 2018-07-24T03:06:24Z

presto-main/src/test/java/com/facebook/presto/sql/query/TestLegacyUnnestArrayRows.java

+    @Test
+    public void testLegacyUnnestArrayRows()
+    {
+        QueryAssertions assertions = new QueryAssertions(testSessionBuilder().build(), new FeaturesConfig().setLegacyUnnestArrayRows(true));


This needs to be closed:

try (QueryAssertions queryAssertions = new QueryAssertions(...)) {

Also, you could just do this instead of adding FeaturesConfig to the QueryAssertions:

testSessionBuilder() .setSystemProperty(LEGACY_UNNEST, "true") .build()

Although, admittedly, that doesn't test the whole config->session->engine path.

martint · 2018-07-24T03:06:43Z

presto-main/src/test/java/com/facebook/presto/sql/query/TestUnnestArrayRows.java

+    @Test
+    public void testUnnestArrayRowsLegacyBehavorDisabled()
+    {
+        QueryAssertions assertions = new QueryAssertions();


martint · 2018-07-24T03:07:20Z

presto-main/src/test/java/com/facebook/presto/sql/query/TestUnnestArrayRows.java

+public class TestUnnestArrayRows
+{
+    @Test
+    public void testUnnestArrayRowsLegacyBehavorDisabled()


I'd just call this testUnnestArrayOfRows

martint · 2018-07-24T03:07:54Z

presto-main/src/test/java/com/facebook/presto/sql/query/TestUnnestArrayRows.java

+
+import org.testng.annotations.Test;
+
+public class TestUnnestArrayRows


I'd call this TestUnnest (this is where all tests for unnest should eventually go)

facebook-github-bot added the CLA Signed label Jun 21, 2018

rongrong requested review from martint and haozhun June 21, 2018 04:35

rongrong force-pushed the unnest_array_row branch 4 times, most recently from 425e667 to df7d46a Compare June 21, 2018 04:51

wenleix reviewed Jun 21, 2018

View reviewed changes

kokosing reviewed Jun 21, 2018

View reviewed changes

rongrong force-pushed the unnest_array_row branch 2 times, most recently from 926d499 to 0f5db0f Compare June 21, 2018 06:55

findepi reviewed Jun 21, 2018

View reviewed changes

rongrong force-pushed the unnest_array_row branch from 0f5db0f to 42ad041 Compare June 21, 2018 17:59

nezihyigitbasi reviewed Jun 21, 2018

View reviewed changes

rongrong force-pushed the unnest_array_row branch from 42ad041 to 0e1314b Compare June 26, 2018 19:45

haozhun suggested changes Jun 26, 2018

View reviewed changes

rongrong force-pushed the unnest_array_row branch from 0e1314b to 851765c Compare June 27, 2018 22:16

rongrong force-pushed the unnest_array_row branch from 851765c to b93a352 Compare July 17, 2018 05:59

martint suggested changes Jul 18, 2018

View reviewed changes

rongrong force-pushed the unnest_array_row branch 2 times, most recently from 440382e to 97179e0 Compare July 21, 2018 00:09

martint approved these changes Jul 24, 2018

View reviewed changes

Unnest array of row to multiple columns

f3f7aa3

rongrong force-pushed the unnest_array_row branch from 97179e0 to f3f7aa3 Compare July 24, 2018 17:42

rongrong closed this Jul 24, 2018

rongrong deleted the unnest_array_row branch July 24, 2018 18:45

rongrong merged commit f3f7aa3 into prestodb:master Jul 24, 2018

oneonestar mentioned this pull request Aug 4, 2018

Preserve the name of column after unnest #11193

Closed

oneonestar mentioned this pull request Sep 6, 2018

WITH ORDINALITY column is unnamed #11429

Closed


		import org.testng.annotations.Test;

		public class TestUnnestArrayRows

Unnest array of row to multiple columns #10883

Unnest array of row to multiple columns #10883

Conversation

rongrong commented Jun 21, 2018

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongrong Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haozhun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongrong commented Jul 23, 2018

martint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongrong Jun 21, 2018 •

edited

Loading

kokosing Jun 21, 2018 •

edited

Loading