[BEAM-2990] support MAP in SQL schema by mingmxu · Pull Request #5079 · apache/beam

mingmxu · 2018-04-10T05:12:35Z

Add type MAP.

Follow this checklist to help us incorporate your contribution quickly and easily:

mingmxu · 2018-04-10T05:14:43Z

R: + @akedin

Can you guys take a look and let's try to finish it before 2.5 cutoff? Thanks!

mingmxu · 2018-04-10T06:13:25Z

retest this please

akedin

Looks good to me overall, few questions:

akedin · 2018-04-10T16:34:30Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

+    // For MAP type, returns the type of the key element.
+    @Nullable public abstract FieldType getComponentKeyType();
+    // For MAP type, returns the type of the value element.
+    @Nullable public abstract FieldType getComponentValueType();


to me it looks like both getComponentValueType() and getComponentType() mean the same thing, i.e. they both describe the type of the value in the container. Keep just one of them?

I would prefer to have different fields for MAP and ARRAY, if it doesn't cause significant performance issue.

akedin · 2018-04-10T16:38:26Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

@@ -222,6 +224,9 @@ public boolean isDateType() {
    public boolean isContainerType() {


Is map a container type as well? A composite type?

while, my thought is MAP-> MAP, ARRAY -> CONTAINER, ROW -> COMPOSITE, to make it clear for the backend types.

mingmxu · 2018-04-10T19:13:34Z

the failure seems unrelated, any tips to handle it?

mingmxu · 2018-04-11T06:21:59Z

retest this please

mingmxu · 2018-04-11T06:50:03Z

run java precommit

reuvenlax · 2018-04-11T10:25:45Z

sdks/java/core/src/test/java/org/apache/beam/sdk/values/RowTest.java

+        put(1, "value1");
+        put(2, "value2");
+        put(3, "value3");
+        put(4, "value4");


I would also add a test where the map value is itself a complex or nested type.

will update to support primitive/array/map/row as value type in Map

reuvenlax · 2018-04-11T10:25:45Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java

+      Map<Object, Object> valueMap = (Map<Object, Object>) value;
+      Map<Object, Object> verifiedMap = Maps.newHashMapWithExpectedSize(valueMap.size());
+      for (Entry<Object, Object> kv : valueMap.entrySet()) {
+        verifiedMap.put(verifyPrimitiveType(kv.getKey(), componentKeyType.getTypeName(), fieldName),


This is actually incorrect right now, since as coded the key type might not be a primitive type. However as I mentioned in Schema.java, I think it would be better to change the key type to be a TypeName, in which case this logic will be correct.

reuvenlax · 2018-04-11T10:25:45Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java

      return this;
    }

+    public <T1, T2> Builder addMap(Map<T1, T2> data) {


Is this overload needed? addArray was needed because otherwise passing an array into addValues tended to unroll the array, but I don't think Java will do that for maps.

it's not necessary, will remove.

reuvenlax · 2018-04-11T10:25:45Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

    public static final Set<TypeName> STRING_TYPES = ImmutableSet.of(STRING);
    public static final Set<TypeName> DATE_TYPES = ImmutableSet.of(DATETIME);
    public static final Set<TypeName> CONTAINER_TYPES = ImmutableSet.of(ARRAY);
+    public static final Set<TypeName> MAP_TYPES = ImmutableSet.of(MAP);


should we consider this part of CONTAINER_TYPES?

would separate here, CONTAINER should be ARRAY/SET. List<KV<>> could be a CONTAINER_TYPE, Map<> is not.

reuvenlax · 2018-04-11T10:25:45Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

+    // For MAP type, returns the type of the key element.
+    @Nullable public abstract FieldType getComponentKeyType();
+    // For MAP type, returns the type of the value element.
+    @Nullable public abstract FieldType getComponentValueType();


XuMingmin wrote:
I would prefer to have different fields for MAP and ARRAY, if it doesn't cause significant performance issue.

two questions/comments

I think the key type be a TypeName instead of a FieldType? Making it a FieldType makes it legal for the key to be a complex value - e.g. they key could be an array type - which doesn't sound very meaningful to me.

Have you considered introducing a new key-value type here (pair of TypeName, FieldType)?

+1, will change to key as primitive, and value can be primitive/array/map/row

reuvenlax · 2018-04-11T10:25:45Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

@@ -222,6 +224,9 @@ public boolean isDateType() {
    public boolean isContainerType() {


XuMingmin wrote:
while, my thought is MAP-> MAP, ARRAY -> CONTAINER, ROW -> COMPOSITE, to make it clear for the backend types.

I think it's actually just another type of container. i.e. one way of imagining .a map is it's just a container of key-value pairs.

mingmxu · 2018-04-12T05:28:56Z

retest this please

mingmxu · 2018-04-13T04:42:44Z

any comments on the change? Would like to close this PR asap as my repository is broken after #4964

mingmxu · 2018-04-13T04:44:12Z

run java precommit

mingmxu · 2018-04-13T07:03:13Z

retest this please

reuvenlax

At sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java:438:

        } else if (TypeName.ROW.equals(componentType.getTypeName())) {

missing Map case.

I think we need to factor this switch out into a helper function used in all these cases, so we don't have to keep updating all of them.

reuvenlax · 2018-04-13T09:45:39Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

    public static final Set<TypeName> COMPOSITE_TYPES = ImmutableSet.of(ROW);

+    public boolean isPrimitiveType() {
+      return isNumericType() || isStringType() || isDateType();


this is not correct (e.g. it excludes boolean). better off making this exclusive (return !isContainterType() && !isCompositeType()).

right, will change

reuvenlax · 2018-04-13T09:45:40Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

    public static final Set<TypeName> STRING_TYPES = ImmutableSet.of(STRING);
    public static final Set<TypeName> DATE_TYPES = ImmutableSet.of(DATETIME);
    public static final Set<TypeName> CONTAINER_TYPES = ImmutableSet.of(ARRAY);
+    public static final Set<TypeName> MAP_TYPES = ImmutableSet.of(MAP);


XuMingmin wrote:
would separate here, CONTAINER should be ARRAY/SET. List<KV<>> could be a CONTAINER_TYPE, Map<> is not.

I'm still confused about this. In most systems, Map is considered a container (e.g. in Java Map is a container type)

In Java, container extends Collection and map extends Map, they're very different IMO. If we merge them together I don't see any benefit as this is a backend function and developers are using either TypeName.ARRAY.type().withComponentType() or TypeName.MAP.type().withMapType.

To make it clear, I would prefer to use the term Collection instead of Component or Contianer. Any comments?

reuvenlax · 2018-04-13T09:45:40Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java

      return this;
    }

+    public <T1, T2> Builder addMap(Map<T1, T2> data) {


XuMingmin wrote:
it's not necessary, will remove.

Acknowledged.

mingmxu · 2018-04-13T19:09:47Z

run java precommit

reuvenlax · 2018-04-14T23:37:48Z

sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/Schema.java

    abstract static class Builder {
      abstract Builder setTypeName(TypeName typeName);
-      abstract Builder setComponentType(@Nullable FieldType componentType);
+      abstract Builder setCollectionType(@Nullable FieldType collectionType);


nitpick here: collectionType reads weird to me, as it seems like it's the type of collection (e.g. array, list, tree, etc.) instead of the type of the component elements of the collection.

Then, what's your prefer?

I didn\t mind componentType, but if you want to include collection in there then maybe CollectionElementType or CollectionValueType?

ok, let me change it to CollectionElementType

ok, LGTM from me once that change is made.

updated, please have a look when you've time.

reuvenlax · 2018-04-16T05:39:41Z

lgtm

mingmxu · 2018-04-16T17:03:53Z

Appreciate @reuvenlax , squash and merging

mingmxu requested review from reuvenlax and xumingming April 10, 2018 05:12

akedin reviewed Apr 10, 2018

View reviewed changes

reuvenlax reviewed Apr 11, 2018

View reviewed changes

mingmxu force-pushed the BEAM-2990 branch 2 times, most recently from 5949dbe to 78f845e Compare April 13, 2018 07:32

reuvenlax reviewed Apr 13, 2018

View reviewed changes

mingmxu added 4 commits April 13, 2018 14:03

support MAP in SQL schema

432979d

in MAP, key as primitive, and value can be primitive/array/map/row

83aa2e4

use Collection for ARRAY type, and re-org verify code in Row

4ec9e60

rebase as file conflict with apache#5089

1fab0a4

mingmxu force-pushed the BEAM-2990 branch from 00841af to 1fab0a4 Compare April 13, 2018 21:09

reuvenlax reviewed Apr 14, 2018

View reviewed changes

rename CollectionType to CollectionElementType

eae842f

mingmxu merged commit 3b3f944 into apache:master Apr 16, 2018

mingmxu deleted the BEAM-2990 branch April 16, 2018 20:52

		@@ -222,6 +224,9 @@ public boolean isDateType() {
		public boolean isContainerType() {

Conversation

mingmxu commented Apr 10, 2018

Uh oh!

mingmxu commented Apr 10, 2018

Uh oh!

mingmxu commented Apr 10, 2018

Uh oh!

akedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmxu commented Apr 10, 2018

Uh oh!

mingmxu commented Apr 11, 2018

Uh oh!

mingmxu commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmxu commented Apr 12, 2018

Uh oh!

mingmxu commented Apr 13, 2018

Uh oh!

mingmxu commented Apr 13, 2018

Uh oh!

mingmxu commented Apr 13, 2018

Uh oh!

reuvenlax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmxu commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reuvenlax commented Apr 16, 2018