[Kernel] Refactor the Array and Map return types in ColumnVector and Row #2087

allisonport-db · 2023-09-22T01:51:31Z

Which Delta project/connector is this regarding?

Description

BASED OFF OF #2069; to review changes in this PR select the last 4 commits (here)

Refactors the ColumnVector and Row getArray and getMap APIs to return wrapper objects. These wrappers provide APIs to retrieve column vector views of the elements or keys/values of the map/array.

How was this patch tested?

Supporting the new APIs in some of the java testing infrastructure was non-trivial, so this PR also moves some tests from TestDeltaTableReads to DeltaTableReadsSuite and adds the complex types to the scala testing infrastructure.

Existing tests have been modified for this PR as well as a few additional checks/tests added.

kernel/kernel-api/src/main/java/io/delta/kernel/internal/data/ScanStateRow.java

...defaults/src/test/java/io/delta/kernel/defaults/internal/parquet/TestParquetBatchReader.java

allisonport-db · 2023-09-22T02:02:29Z

kernel/kernel-defaults/src/test/java/io/delta/kernel/defaults/utils/DefaultKernelTestUtils.java

+     * Get the value at given {@code rowId} from the column vector. The type of the value object
+     * depends on the data type of the {@code vector}.
+     */
+    public static Object getValueAsObject(ColumnVector vector, int rowId) {


Moved over from kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/data/vector/VectorUtils.java since it was only being used in test code after these changes

kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/data/DefaultJsonRow.java

scottsand-db

Looks good! Left some comments

scottsand-db · 2023-09-23T01:07:23Z

kernel/kernel-api/src/main/java/io/delta/kernel/data/Row.java

@@ -112,11 +110,11 @@ public interface Row {
     * Return array value of the column located at the given ordinal.
     * Throws error if the column at given ordinal is not of array type,
     */
-    <T> List<T> getArray(int ordinal);
+    ArrayValue getArray(int ordinal);


Would getArrayValue and getMapValue be better APIs?

Since getMap makes me think it will return a java.util.Map

I'm not sure I necessarily agree with

getMap makes me think it will return a java.util.Map

But I'm relatively indifferent about changing this if everyone is in agreement. Noting that Spark's method is ColumnVector::getMap --> ColumnarMap.

Let's use getArrayValue etc. Map and Array ubiquitously refer to java.util.Map and the java array type

Let's followup on this when we do the API review (easy to change in a followup PR)

kernel/kernel-api/src/main/java/io/delta/kernel/internal/ScanBuilderImpl.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/data/ScanStateRow.java

scottsand-db · 2023-09-23T01:18:49Z

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Metadata.java

        return configuration;
    }
+
+    public String getColumnMappingMode() {


hm. I don't know if this is a very scalable or future-proof design.

we won't have getters for every other configuration, will we? (I don't think so)

So, what makes this one special?

Instead, we should just lazily convert the configuration MapValue into a Map on the first .getTableProperty(key: String) and access it that way

Added getConfiguration --> Map<String, String> instead. Let me know if you'd prefer a getTableProperty API instead, but I think we'd have to provide a couple methods

boolean isTablePropertySet(key)

String getTableProperty(key)

String getTableProperty(key, default) <-- optional

kernel/kernel-api/src/main/java/io/delta/kernel/utils/VectorUtils.java

kernel/kernel-api/src/main/java/io/delta/kernel/internal/data/ScanStateRow.java

kernel/kernel-api/src/main/java/io/delta/kernel/utils/ArrayValueWrapper.java

kernel/kernel-api/src/main/java/io/delta/kernel/utils/VectorUtils.java

vkorukanti · 2023-09-23T06:24:27Z

kernel/kernel-api/src/test/java/io/delta/kernel/internal/types/JsonHandlerTestImpl.java

@@ -160,7 +157,17 @@ private static Object decodeElement(JsonNode jsonValue, DataType dataType) {
                    final Object parsedElement = decodeElement(element, arrayType.getElementType());
                    output.add(parsedElement);
                }
-                return output;
+                return new ArrayValue() {


If this becomes a common pattern, add a utility method to convert List to ArrayValue.

vkorukanti · 2023-09-23T06:27:49Z

kernel/kernel-api/src/test/java/io/delta/kernel/internal/types/JsonHandlerTestImpl.java

        }

        @Override
        public <K, V> Map<K, V> getMap(int ordinal) {
            return (Map<K, V>) parsedValues[ordinal];
        }
    }
+
+    private static class TestColumnVector implements ColumnVector {


Wondering if there is another alternative to this. Does Spark has similar requirement where they need to expose a list as Vector?

...defaults/src/test/java/io/delta/kernel/defaults/internal/parquet/TestParquetBatchReader.java

vkorukanti · 2023-09-23T06:34:42Z

kernel/kernel-api/src/main/java/io/delta/kernel/utils/VectorUtils.java

@@ -30,7 +30,8 @@ public final class VectorUtils {
    private VectorUtils() {}


Can we consolidate the toJavaMap(MapValue) and toMapValue(java.util.Map here? Similar for the array.

Responding to the (I think) 3 comments related to this here.

We need to expose java list/maps as MapValue/ArrayValue (and thus as ColumnVectors) in a couple of places

In kernelApi:

(1) JsonHandlerTestImpl

In kernelDefaults:

(2) DefaultJsonHandlerImpl (in DefaultJsonRow) -- very similar pattern to the first

(3) Tests (once in ExpressionEvaluatorSuite)

I can add a test utility to do this to cover (1) and (3) but I don't think we want to add this to VectorUtils as we shouldn't be creating ColumnVectors there?

How about add utility method for defaults and it can be used in default prod code and test code.

For the JsonHandlerTestImpl, I am actually thinking of removing it and make it a proper mock.

Added a DefaultGenericVector and use that in DefaultJsonRow and in a test utility I added that converts a Seq[Map[..,..]]to a CV. Can't really combine more than that since in DefaultJsonRow we can build the keys/values separately and don't start with a Map

vkorukanti

Please rebase the change and will take another pass.

allisonport-db · 2023-09-28T23:50:52Z

...defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/ElementAtEvaluator.java

+                                mapValue.getValues().getString(i);
+                    }
+                }
+                // TODO what is the behavior for element_at if not in the map?


@vkorukanti I can update the docs for the function as well

We shouldn't return null. We should return an Optional.

What if the value for the key is null? Isn't that perfectly valid?

Thank you! Please add the docs. I didn't mention the null behavior. The behavior should be it should return null. We have tests that confirm this.

...st/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala

scottsand-db · 2023-09-29T16:39:04Z

kernel/kernel-api/src/main/java/io/delta/kernel/data/Row.java

@@ -112,11 +110,11 @@ public interface Row {
     * Return array value of the column located at the given ordinal.
     * Throws error if the column at given ordinal is not of array type,
     */
-    <T> List<T> getArray(int ordinal);
+    ArrayValue getArray(int ordinal);


Let's use getArrayValue etc. Map and Array ubiquitously refer to java.util.Map and the java array type

kernel/kernel-api/src/main/java/io/delta/kernel/internal/actions/Metadata.java

kernel/kernel-api/src/main/java/io/delta/kernel/utils/VectorUtils.java

...defaults/src/main/java/io/delta/kernel/defaults/internal/data/vector/DefaultArrayVector.java

scottsand-db · 2023-09-29T17:03:03Z

...defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/ElementAtEvaluator.java

+                                mapValue.getValues().getString(i);
+                    }
+                }
+                // TODO what is the behavior for element_at if not in the map?


We shouldn't return null. We should return an Optional.

What if the value for the key is null? Isn't that perfectly valid?

...defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/ElementAtEvaluator.java

scottsand-db · 2023-09-29T17:05:19Z

...defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/ElementAtEvaluator.java

@@ -73,7 +74,7 @@ static ColumnVector eval(ColumnVector map, ColumnVector lookupKey) {
            // The general pattern is call `isNullAt(rowId)` followed by `getString`.
            // So the cache of one value is enough.
            private int lastLookupRowId = -1;
-            private Object lastLookupValue = null;
+            private String lastLookupValue = null;


what does null represent? that there was no last lookup value? this is ambiguous, it should be Optional

Can't represent a null value if we make it Optional<String>

I think lastLookupRowId reflects whether there was a last lookup value.

kernel/kernel-api/src/main/java/io/delta/kernel/data/ArrayValue.java

kernel/kernel-api/src/main/java/io/delta/kernel/data/MapValue.java

vkorukanti · 2023-09-29T17:06:16Z

...-defaults/src/main/java/io/delta/kernel/defaults/internal/data/vector/DefaultViewVector.java

+
+    @Override
+    public void close() {
+        underlyingVector.close();


This could be a problem. We shouldn't be closing the underlying vector as it could still be used.

For Java heap-based vectors this doesn't matter, for off-heap-based ones, we need some kind of ref counting.

vkorukanti · 2023-09-29T17:06:53Z

...defaults/src/main/java/io/delta/kernel/defaults/internal/expressions/ElementAtEvaluator.java

+                                mapValue.getValues().getString(i);
+                    }
+                }
+                // TODO what is the behavior for element_at if not in the map?


Thank you! Please add the docs. I didn't mention the null behavior. The behavior should be it should return null. We have tests that confirm this.

tdas · 2023-09-29T18:59:01Z

This PR has progressed enough that its not great to change it right now, but it would have been better to split this large PR into 2 PRs one for Array and one for Map

allisonport-db · 2023-09-29T20:34:56Z

This PR has progressed enough that its not great to change it right now, but it would have been better to split this large PR into 2 PRs one for Array and one for Map

@tdas Tried to do this originally but it breaks a bunch of the existing tests, and couldn't refactor them to the new framework without making the changes for both types.

…Row (delta-io#2087) #### Which Delta project/connector is this regarding?  - [ ] Spark - [ ] Standalone - [ ] Flink - [x] Kernel - [ ] Other (fill in here) ## Description BASED OFF OF delta-io#2069; to review changes in this PR select the last 4 commits ([here](https://github.com/delta-io/delta/pull/2087/files/a5073b4c14496314bae1ca97163f9e7bbc67bc6d..d160915717d683ebfd6087a93ccb224a57216d82)) Refactors the ColumnVector and Row `getArray` and `getMap` APIs to return wrapper objects. These wrappers provide APIs to retrieve column vector views of the elements or keys/values of the map/array. ## How was this patch tested? Supporting the new APIs in some of the java testing infrastructure was non-trivial, so this PR also moves some tests from TestDeltaTableReads to DeltaTableReadsSuite and adds the complex types to the scala testing infrastructure. Existing tests have been modified for this PR as well as a few additional checks/tests added.

allisonport-db commented Sep 22, 2023

View reviewed changes

kernel/kernel-api/src/main/java/io/delta/kernel/internal/data/ScanStateRow.java Show resolved Hide resolved

allisonport-db commented Sep 22, 2023

View reviewed changes

...defaults/src/test/java/io/delta/kernel/defaults/internal/parquet/TestParquetBatchReader.java Outdated Show resolved Hide resolved

allisonport-db commented Sep 22, 2023

View reviewed changes

allisonport-db requested review from vkorukanti and scottsand-db September 22, 2023 02:43

allisonport-db commented Sep 22, 2023

View reviewed changes

kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/data/DefaultJsonRow.java Outdated Show resolved Hide resolved

scottsand-db reviewed Sep 23, 2023

View reviewed changes

vkorukanti reviewed Sep 23, 2023

View reviewed changes

vkorukanti approved these changes Sep 23, 2023

View reviewed changes

vkorukanti self-requested a review September 23, 2023 06:36

vkorukanti reviewed Sep 23, 2023

View reviewed changes

allisonport-db added 4 commits September 28, 2023 13:25

initial changes for Array support

372df70

initial map impl

40db527

clean up

b615cfe

checkstyle fix

d5b7d30

vkorukanti mentioned this pull request Sep 28, 2023

[Umbrella Feature Request] Delta Kernel APIs to simplify building connectors for reading Delta tables #1783

Open

3 tasks

Rebase & respond to some comments

f72d5fb

allisonport-db force-pushed the complex-type branch from d160915 to f72d5fb Compare September 28, 2023 22:49

allisonport-db commented Sep 28, 2023

View reviewed changes

instantiate DefaultViewVector once

a494ac4

allisonport-db requested review from vkorukanti and scottsand-db September 28, 2023 23:55

allisonport-db commented Sep 28, 2023

View reviewed changes

...st/scala/io/delta/kernel/defaults/internal/expressions/DefaultExpressionEvaluatorSuite.scala Outdated Show resolved Hide resolved

scottsand-db reviewed Sep 29, 2023

View reviewed changes

vkorukanti approved these changes Sep 29, 2023

View reviewed changes

allisonport-db added 3 commits October 1, 2023 17:10

respond to comments

db60b18

Respond to more comments

9da6b8d

docs

c605d27

fix scala style

e24f8c1

allisonport-db merged commit a038186 into delta-io:master Oct 2, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kernel] Refactor the Array and Map return types in ColumnVector and Row #2087

[Kernel] Refactor the Array and Map return types in ColumnVector and Row #2087

allisonport-db commented Sep 22, 2023 •

edited

Loading

allisonport-db Sep 22, 2023

scottsand-db left a comment

scottsand-db Sep 23, 2023

allisonport-db Sep 28, 2023

scottsand-db Sep 29, 2023

allisonport-db Oct 2, 2023 •

edited

Loading

scottsand-db Sep 23, 2023

allisonport-db Sep 28, 2023

vkorukanti Sep 23, 2023

vkorukanti Sep 23, 2023

vkorukanti Sep 23, 2023

allisonport-db Sep 28, 2023

vkorukanti Sep 29, 2023

allisonport-db Oct 2, 2023

vkorukanti left a comment

allisonport-db Sep 28, 2023

scottsand-db Sep 29, 2023

vkorukanti Sep 29, 2023

scottsand-db Sep 29, 2023

scottsand-db Sep 29, 2023

scottsand-db Sep 29, 2023

allisonport-db Oct 2, 2023

allisonport-db Oct 2, 2023

vkorukanti Sep 29, 2023

vkorukanti Sep 29, 2023

tdas commented Sep 29, 2023

allisonport-db commented Sep 29, 2023

		@@ -30,7 +30,8 @@ public final class VectorUtils {
		private VectorUtils() {}

[Kernel] Refactor the Array and Map return types in ColumnVector and Row #2087

[Kernel] Refactor the Array and Map return types in ColumnVector and Row #2087

Conversation

allisonport-db commented Sep 22, 2023 • edited Loading

Which Delta project/connector is this regarding?

Description

How was this patch tested?

Choose a reason for hiding this comment

scottsand-db 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

allisonport-db Oct 2, 2023 • 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

vkorukanti 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdas commented Sep 29, 2023

allisonport-db commented Sep 29, 2023

allisonport-db commented Sep 22, 2023 •

edited

Loading

allisonport-db Oct 2, 2023 •

edited

Loading