You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/01/10 00:21:57 UTC
[GitHub] [arrow] nbruno opened a new pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
nbruno opened a new pull request #9151:
URL: https://github.com/apache/arrow/pull/9151
This pull request adds support for Map types in FieldReader and FieldWriter.
Initial unit tests show how to add the following nested types:
- A list of maps: `List<Map<Integer, Long>>`
- Nesting a map as the value of another map: `Map<Long, Map<Long, Long>>`
- Nesting maps as both the key and value of another map: `Map<Map<Integer, Integer>, Map<Long, Long>>`
Appreciate any feedback or suggestions for improvement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r612785178
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,41 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ return map(name, false);
+ }
+
+ @Override
+ public MapWriter map(String name, boolean keysSorted) {
+ FieldWriter writer = fields.get(handleCase(name));
+ if(writer == null) {
+ ValueVector vector;
+ ValueVector currentVector = container.getChild(name);
+ MapVector v = container.addOrGet(name,
+ new FieldType(addVectorAsNullable,
+ new ArrowType.Map(keysSorted)
+ ,null, null),
+ MapVector.class);
+ writer = new PromotableWriter(v, container, getNullableStructWriterFactory());
+ vector = v;
+ if (currentVector == null || currentVector != vector) {
+ if(this.initialCapacity > 0) {
+ vector.setInitialCapacity(this.initialCapacity);
+ }
+ vector.allocateNewSafe();
+ }
+ writer.setPosition(idx());
+ fields.put(handleCase(name), writer);
+ } else {
+ if (writer instanceof PromotableWriter) {
Review comment:
This is following the same style as the other methods in this class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565605138
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -64,6 +64,11 @@
case LIST:
list(child.getName());
break;
+ case MAP:{
+ org.apache.arrow.vector.types.pojo.ArrowType.Map arrowType = (org.apache.arrow.vector.types.pojo.ArrowType.Map) child.getType();
Review comment:
Done, I think I saw somewhere else fully-qualifying these imports, but yes it's imported and unnecessary, so I removed it.
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name, boolean keysSorted) {
+ FieldWriter writer = fields.get(handleCase(name));
+ if(writer == null) {
+ ValueVector vector;
+ ValueVector currentVector = container.getChild(name);
+ MapVector v = container.addOrGet(name,
+ new FieldType(addVectorAsNullable,
+ new org.apache.arrow.vector.types.pojo.ArrowType.Map(keysSorted)
Review comment:
Done
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
##########
@@ -546,6 +548,54 @@ private void checkUnionList(ListVector listVector) {
}
}
+ @Test
+ public void testListMapType() {
+ try (ListVector listVector = ListVector.empty("list", allocator)) {
+ listVector.allocateNew();
+ UnionListWriter listWriter = new UnionListWriter(listVector);
+ MapWriter innerMapWriter = listWriter.map(true);
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r582507357
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(1);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(50);
+ valueWriter.value().bigInt().writeBigInt(100);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(200);
+ valueWriter.value().bigInt().writeBigInt(400);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(2);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(75);
+ valueWriter.value().bigInt().writeBigInt(175);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(150);
+ valueWriter.value().bigInt().writeBigInt(250);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ /* write one or more maps at index 1 */
+ mapWriter.setPosition(1);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(3);
+ valueWriter = mapWriter.value().map(true);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(10);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(4);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(15);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(5);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(25);
+ valueWriter.value().bigInt().writeBigInt(30);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(35);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ assertEquals(1, mapVector.getLastSet());
+
+ mapWriter.setValueCount(2);
+
+ assertEquals(2, mapVector.getValueCount());
+
+ // Get mapVector element at index 0
+ Object result = mapVector.getObject(0);
+ ArrayList<?> resultSet = (ArrayList<?>) result;
+
+ // 2 map entries at index 0
+ assertEquals(2, resultSet.size());
+
+ // First Map entry
+ Map<?, ?> resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(1L, getResultKey(resultStruct));
+ ArrayList<Map<?, ?>> list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of 2 two maps
+ Map<?, ?> innerMap = list.get(0);
+ assertEquals(50L, getResultKey(innerMap));
+ assertEquals(100L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(200L, getResultKey(innerMap));
+ assertEquals(400L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(2L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of two maps
+ innerMap = list.get(0);
+ assertEquals(75L, getResultKey(innerMap));
+ assertEquals(175L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(150L, getResultKey(innerMap));
+ assertEquals(250L, getResultValue(innerMap));
+
+ // Get mapVector element at index 1
+ result = mapVector.getObject(1);
+ resultSet = (ArrayList<?>) result;
+
+ // 3 map entries at index 1
+ assertEquals(3, resultSet.size());
+
+ // First Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(3L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(10L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(4L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(15L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Third Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(2);
+ assertEquals(5L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of maps with 2 elements
+ innerMap = list.get(0);
+ assertEquals(25L, getResultKey(innerMap));
+ assertEquals(30L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(35L, getResultKey(innerMap));
+ assertNull(innerMap.get(MapVector.VALUE_NAME));
+
+ /* check underlying bitVector */
+ assertFalse(mapVector.isNull(0));
Review comment:
Done. MapVector doesn't support null for a key, but there are entries where the value is null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] siddharthteotia commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r566552082
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(1);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(50);
+ valueWriter.value().bigInt().writeBigInt(100);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(200);
+ valueWriter.value().bigInt().writeBigInt(400);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(2);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(75);
+ valueWriter.value().bigInt().writeBigInt(175);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(150);
+ valueWriter.value().bigInt().writeBigInt(250);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ /* write one or more maps at index 1 */
+ mapWriter.setPosition(1);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(3);
+ valueWriter = mapWriter.value().map(true);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(10);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(4);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(15);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(5);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(25);
+ valueWriter.value().bigInt().writeBigInt(30);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(35);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ assertEquals(1, mapVector.getLastSet());
+
+ mapWriter.setValueCount(2);
+
+ assertEquals(2, mapVector.getValueCount());
+
+ // Get mapVector element at index 0
+ Object result = mapVector.getObject(0);
+ ArrayList<?> resultSet = (ArrayList<?>) result;
+
+ // 2 map entries at index 0
+ assertEquals(2, resultSet.size());
+
+ // First Map entry
+ Map<?, ?> resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(1L, getResultKey(resultStruct));
+ ArrayList<Map<?, ?>> list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of 2 two maps
+ Map<?, ?> innerMap = list.get(0);
+ assertEquals(50L, getResultKey(innerMap));
+ assertEquals(100L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(200L, getResultKey(innerMap));
+ assertEquals(400L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(2L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of two maps
+ innerMap = list.get(0);
+ assertEquals(75L, getResultKey(innerMap));
+ assertEquals(175L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(150L, getResultKey(innerMap));
+ assertEquals(250L, getResultValue(innerMap));
+
+ // Get mapVector element at index 1
+ result = mapVector.getObject(1);
+ resultSet = (ArrayList<?>) result;
+
+ // 3 map entries at index 1
+ assertEquals(3, resultSet.size());
+
+ // First Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(3L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(10L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(4L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(15L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Third Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(2);
+ assertEquals(5L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of maps with 2 elements
+ innerMap = list.get(0);
+ assertEquals(25L, getResultKey(innerMap));
+ assertEquals(30L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(35L, getResultKey(innerMap));
+ assertNull(innerMap.get(MapVector.VALUE_NAME));
+
+ /* check underlying bitVector */
+ assertFalse(mapVector.isNull(0));
Review comment:
Can we add a null element at the root position and also inside the nested map?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-785554910
I updated the PR with the requested changes. Please let me know if there are other issues / concerns. @BryanCutler @liyafan82 @siddharthteotia
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565794265
##########
File path: java/vector/src/main/codegen/templates/UnionVector.java
##########
@@ -647,6 +673,8 @@ public ValueVector getVectorByType(int typeId, ArrowType arrowType) {
return getStruct();
case LIST:
return getList();
+ case MAP:
+ return getMap();
Review comment:
here we should call `getMap(arrowType)`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-763085859
Thanks for the PR @nbruno! I'm a little backlogged at the moment, but will review as soon as I can.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r564772738
##########
File path: java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
##########
@@ -144,6 +179,16 @@ public ListWriter list() {
return getWriter(MinorType.LIST).list();
}
+ @Override
+ public MapWriter map() {
+ return getWriter(MinorType.LIST).map();
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ return getWriter(MinorType.MAP, new org.apache.arrow.vector.types.pojo.ArrowType.Map(keysSorted));
Review comment:
you should just be able to do `ArrowType.Map(keysSorted)`, it's imported already
##########
File path: java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
##########
@@ -44,7 +44,11 @@
* @param type the type of the values we want to write
Review comment:
Could you remove the imports from line 21 and above while we are here, they aren't used.
https://github.com/apache/arrow/pull/9151/files#diff-d8951dca853745d4e19fc70856ff955b02ffe333cdcd34bb2ce02c1c83c4f1fcR21
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
##########
@@ -546,6 +548,54 @@ private void checkUnionList(ListVector listVector) {
}
}
+ @Test
+ public void testListMapType() {
+ try (ListVector listVector = ListVector.empty("list", allocator)) {
+ listVector.allocateNew();
+ UnionListWriter listWriter = new UnionListWriter(listVector);
+ MapWriter innerMapWriter = listWriter.map(true);
Review comment:
can you verify the final vector has attribute `keysSorted` to `true`?
##########
File path: java/vector/src/main/codegen/templates/UnionWriter.java
##########
@@ -110,6 +143,26 @@ public ListWriter asList() {
return getListWriter();
}
+ private MapWriter getMapWriter() {
+ // returns existing writer
+ Preconditions.checkNotNull(mapWriter);
Review comment:
Should this also create a writer if there is no existing?
##########
File path: java/vector/src/main/codegen/templates/UnionFixedSizeListWriter.java
##########
@@ -169,6 +169,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
should be this ?
```Java
MapWriter mapWriter = writer.map(keysSorted);
return mapWriter;
```
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name, boolean keysSorted) {
+ FieldWriter writer = fields.get(handleCase(name));
+ if(writer == null) {
+ ValueVector vector;
+ ValueVector currentVector = container.getChild(name);
+ MapVector v = container.addOrGet(name,
+ new FieldType(addVectorAsNullable,
+ new org.apache.arrow.vector.types.pojo.ArrowType.Map(keysSorted)
Review comment:
can use `ArrowType.Map`
##########
File path: java/vector/src/main/codegen/templates/UnionListWriter.java
##########
@@ -176,6 +176,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
same here, return the new `mapWriter`?
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
Review comment:
why this precondition? It seems other places it will create a writer if NULL?
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -64,6 +64,11 @@
case LIST:
list(child.getName());
break;
+ case MAP:{
+ org.apache.arrow.vector.types.pojo.ArrowType.Map arrowType = (org.apache.arrow.vector.types.pojo.ArrowType.Map) child.getType();
Review comment:
Should be able to use `ArrowType.Map`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-819602113
@nbruno thank you! @BryanCutler did you have any more concerns. I'll do a quick scan and merge tonight if not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] siddharthteotia commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-769545706
I reviewed the tests. Please give me a day to finish rest of the review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565615777
##########
File path: java/vector/src/main/codegen/templates/UnionFixedSizeListWriter.java
##########
@@ -169,6 +169,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
I had originally thought this too, but making this change will cause the new unit tests I added for MapWriter to break (it won't correctly write a list of maps).
I believe it is because of how List and Map vectors are implemented. Map itself does not store any data, it is a subclass of List, delegating storage to the parent. When we have a list and want to write a map value to get `List<Map<?, ?>>`, we don't need a new writer with a new vector to write into, we actually want to reuse the vector of the current list. If we construct a new writer and return that, we would end up with `List<List<Map<?, ?>>>` when we really want `List<Map<?,?>>`.
My explanation probably isn't 100% clear or correct, but it's how I was able to figure out how to implement MapWriter for this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-770457799
@siddharthteotia did you finish up your review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] siddharthteotia commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r566552082
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(1);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(50);
+ valueWriter.value().bigInt().writeBigInt(100);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(200);
+ valueWriter.value().bigInt().writeBigInt(400);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(2);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(75);
+ valueWriter.value().bigInt().writeBigInt(175);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(150);
+ valueWriter.value().bigInt().writeBigInt(250);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ /* write one or more maps at index 1 */
+ mapWriter.setPosition(1);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(3);
+ valueWriter = mapWriter.value().map(true);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(10);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(4);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(15);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(5);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(25);
+ valueWriter.value().bigInt().writeBigInt(30);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(35);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ assertEquals(1, mapVector.getLastSet());
+
+ mapWriter.setValueCount(2);
+
+ assertEquals(2, mapVector.getValueCount());
+
+ // Get mapVector element at index 0
+ Object result = mapVector.getObject(0);
+ ArrayList<?> resultSet = (ArrayList<?>) result;
+
+ // 2 map entries at index 0
+ assertEquals(2, resultSet.size());
+
+ // First Map entry
+ Map<?, ?> resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(1L, getResultKey(resultStruct));
+ ArrayList<Map<?, ?>> list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of 2 two maps
+ Map<?, ?> innerMap = list.get(0);
+ assertEquals(50L, getResultKey(innerMap));
+ assertEquals(100L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(200L, getResultKey(innerMap));
+ assertEquals(400L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(2L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of two maps
+ innerMap = list.get(0);
+ assertEquals(75L, getResultKey(innerMap));
+ assertEquals(175L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(150L, getResultKey(innerMap));
+ assertEquals(250L, getResultValue(innerMap));
+
+ // Get mapVector element at index 1
+ result = mapVector.getObject(1);
+ resultSet = (ArrayList<?>) result;
+
+ // 3 map entries at index 1
+ assertEquals(3, resultSet.size());
+
+ // First Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(3L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(10L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(4L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(15L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Third Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(2);
+ assertEquals(5L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of maps with 2 elements
+ innerMap = list.get(0);
+ assertEquals(25L, getResultKey(innerMap));
+ assertEquals(30L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(35L, getResultKey(innerMap));
+ assertNull(innerMap.get(MapVector.VALUE_NAME));
+
+ /* check underlying bitVector */
+ assertFalse(mapVector.isNull(0));
Review comment:
Can we add a null element at the root position and also inside the map?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jjbskir commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
jjbskir commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-760227433
You should add a section to the [Java docs](https://github.com/apache/arrow/tree/master/docs/source/java) so people know to find this feature!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565604839
##########
File path: java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
##########
@@ -144,6 +179,16 @@ public ListWriter list() {
return getWriter(MinorType.LIST).list();
}
+ @Override
+ public MapWriter map() {
+ return getWriter(MinorType.LIST).map();
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ return getWriter(MinorType.MAP, new org.apache.arrow.vector.types.pojo.ArrowType.Map(keysSorted));
Review comment:
Done
##########
File path: java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
##########
@@ -44,7 +44,11 @@
* @param type the type of the values we want to write
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r566226822
##########
File path: java/vector/src/main/codegen/templates/UnionVector.java
##########
@@ -647,6 +673,8 @@ public ValueVector getVectorByType(int typeId, ArrowType arrowType) {
return getStruct();
case LIST:
return getList();
+ case MAP:
+ return getMap();
Review comment:
I wonder if it should be `getMap(name, arrowType)` just like decimal
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r570608317
##########
File path: java/vector/src/main/codegen/templates/UnionFixedSizeListWriter.java
##########
@@ -169,6 +169,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
It should work pretty much like `ListWriter` and this isn't how `ListWriter` is done in this class, so something doesn't seem quite right, but I could be wrong. I'll try to look into it and see what's going on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] siddharthteotia commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r566552494
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(1);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(50);
+ valueWriter.value().bigInt().writeBigInt(100);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(200);
+ valueWriter.value().bigInt().writeBigInt(400);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(2);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(75);
+ valueWriter.value().bigInt().writeBigInt(175);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(150);
+ valueWriter.value().bigInt().writeBigInt(250);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ /* write one or more maps at index 1 */
+ mapWriter.setPosition(1);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(3);
+ valueWriter = mapWriter.value().map(true);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(10);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(4);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(15);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(5);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(25);
+ valueWriter.value().bigInt().writeBigInt(30);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(35);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ assertEquals(1, mapVector.getLastSet());
+
+ mapWriter.setValueCount(2);
+
+ assertEquals(2, mapVector.getValueCount());
+
+ // Get mapVector element at index 0
+ Object result = mapVector.getObject(0);
+ ArrayList<?> resultSet = (ArrayList<?>) result;
+
+ // 2 map entries at index 0
+ assertEquals(2, resultSet.size());
+
+ // First Map entry
+ Map<?, ?> resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(1L, getResultKey(resultStruct));
+ ArrayList<Map<?, ?>> list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of 2 two maps
+ Map<?, ?> innerMap = list.get(0);
+ assertEquals(50L, getResultKey(innerMap));
+ assertEquals(100L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(200L, getResultKey(innerMap));
+ assertEquals(400L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(2L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of two maps
+ innerMap = list.get(0);
+ assertEquals(75L, getResultKey(innerMap));
+ assertEquals(175L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(150L, getResultKey(innerMap));
+ assertEquals(250L, getResultValue(innerMap));
+
+ // Get mapVector element at index 1
+ result = mapVector.getObject(1);
+ resultSet = (ArrayList<?>) result;
+
+ // 3 map entries at index 1
+ assertEquals(3, resultSet.size());
+
+ // First Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(3L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(10L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(4L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(15L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Third Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(2);
+ assertEquals(5L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of maps with 2 elements
+ innerMap = list.get(0);
+ assertEquals(25L, getResultKey(innerMap));
+ assertEquals(30L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(35L, getResultKey(innerMap));
+ assertNull(innerMap.get(MapVector.VALUE_NAME));
+
+ /* check underlying bitVector */
+ assertFalse(mapVector.isNull(0));
+ assertFalse(mapVector.isNull(1));
+
+ /* check underlying offsets */
+ final ArrowBuf offsetBuffer = mapVector.getOffsetBuffer();
+
+ /* mapVector has 2 entries at index 0 and 3 entries at index 1 */
+ assertEquals(0, offsetBuffer.getInt(0 * MapVector.OFFSET_WIDTH));
+ assertEquals(2, offsetBuffer.getInt(1 * MapVector.OFFSET_WIDTH));
+ assertEquals(5, offsetBuffer.getInt(2 * MapVector.OFFSET_WIDTH));
+ }
+ }
+
+ @Test
+ public void testMapWithMapKeyAndMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter keyWriter;
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Map<Integer, Integer>, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
Review comment:
I think is going to be hard to debug without actually having a description of what the map will look like finally. So would be great to add that in comments. Also, you should consider extracting this into a function and pass all the values and writers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r570608317
##########
File path: java/vector/src/main/codegen/templates/UnionFixedSizeListWriter.java
##########
@@ -169,6 +169,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
It should work pretty much like `ListWriter` and this isn't how `ListWriter` is done in this class, so something doesn't seem quite right, but I could be wrong. I'll try to look into it and see what's going on.
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
Review comment:
We should probably have the writer default to `keysSorted=false` since that will be the most common usage and would only be used if someone really needed this flag. Would that allow you to change this to create a writer by default then, and only supply `keysSorted` optionally?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r612784964
##########
File path: java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
##########
@@ -144,6 +174,16 @@ public ListWriter list() {
return getWriter(MinorType.LIST).list();
}
+ @Override
+ public MapWriter map() {
+ return getWriter(MinorType.LIST).map();
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ return getWriter(MinorType.MAP, new ArrowType.Map(keysSorted));
Review comment:
This would probably make the most sense, I'm just not sure it should be addressed in this PR or as a separate issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-757389014
https://issues.apache.org/jira/browse/ARROW-11173
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565796770
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
Review comment:
It would be helpful to extract such code into a common method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] siddharthteotia commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r566548084
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
Review comment:
What would help (especially the people who will edit this test in the future) is to have a description in comments of how the map will look like after you have added whatever data you want to use for test. I had used this in several places in TestValueVector, TestListVector etc and it helps a lot with visualizing and debugging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r582508539
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(1);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(50);
+ valueWriter.value().bigInt().writeBigInt(100);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(200);
+ valueWriter.value().bigInt().writeBigInt(400);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(2);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(75);
+ valueWriter.value().bigInt().writeBigInt(175);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(150);
+ valueWriter.value().bigInt().writeBigInt(250);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ /* write one or more maps at index 1 */
+ mapWriter.setPosition(1);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(3);
+ valueWriter = mapWriter.value().map(true);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(10);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(4);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(15);
+ valueWriter.value().bigInt().writeBigInt(20);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.startEntry();
+ mapWriter.key().bigInt().writeBigInt(5);
+ valueWriter = mapWriter.value().map(false);
+ valueWriter.startMap();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(25);
+ valueWriter.value().bigInt().writeBigInt(30);
+ valueWriter.endEntry();
+ valueWriter.startEntry();
+ valueWriter.key().bigInt().writeBigInt(35);
+ valueWriter.endEntry();
+ valueWriter.endMap();
+ mapWriter.endEntry();
+
+ mapWriter.endMap();
+
+ assertEquals(1, mapVector.getLastSet());
+
+ mapWriter.setValueCount(2);
+
+ assertEquals(2, mapVector.getValueCount());
+
+ // Get mapVector element at index 0
+ Object result = mapVector.getObject(0);
+ ArrayList<?> resultSet = (ArrayList<?>) result;
+
+ // 2 map entries at index 0
+ assertEquals(2, resultSet.size());
+
+ // First Map entry
+ Map<?, ?> resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(1L, getResultKey(resultStruct));
+ ArrayList<Map<?, ?>> list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of 2 two maps
+ Map<?, ?> innerMap = list.get(0);
+ assertEquals(50L, getResultKey(innerMap));
+ assertEquals(100L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(200L, getResultKey(innerMap));
+ assertEquals(400L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(2L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of two maps
+ innerMap = list.get(0);
+ assertEquals(75L, getResultKey(innerMap));
+ assertEquals(175L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(150L, getResultKey(innerMap));
+ assertEquals(250L, getResultValue(innerMap));
+
+ // Get mapVector element at index 1
+ result = mapVector.getObject(1);
+ resultSet = (ArrayList<?>) result;
+
+ // 3 map entries at index 1
+ assertEquals(3, resultSet.size());
+
+ // First Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(0);
+ assertEquals(3L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(10L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Second Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(1);
+ assertEquals(4L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(1, list.size()); // value is a list of maps with 1 element
+ innerMap = list.get(0);
+ assertEquals(15L, getResultKey(innerMap));
+ assertEquals(20L, getResultValue(innerMap));
+
+ // Third Map entry
+ resultStruct = (Map<?, ?>) resultSet.get(2);
+ assertEquals(5L, getResultKey(resultStruct));
+ list = (ArrayList<Map<?, ?>>) getResultValue(resultStruct);
+ assertEquals(2, list.size()); // value is a list of maps with 2 elements
+ innerMap = list.get(0);
+ assertEquals(25L, getResultKey(innerMap));
+ assertEquals(30L, getResultValue(innerMap));
+ innerMap = list.get(1);
+ assertEquals(35L, getResultKey(innerMap));
+ assertNull(innerMap.get(MapVector.VALUE_NAME));
+
+ /* check underlying bitVector */
+ assertFalse(mapVector.isNull(0));
+ assertFalse(mapVector.isNull(1));
+
+ /* check underlying offsets */
+ final ArrowBuf offsetBuffer = mapVector.getOffsetBuffer();
+
+ /* mapVector has 2 entries at index 0 and 3 entries at index 1 */
+ assertEquals(0, offsetBuffer.getInt(0 * MapVector.OFFSET_WIDTH));
+ assertEquals(2, offsetBuffer.getInt(1 * MapVector.OFFSET_WIDTH));
+ assertEquals(5, offsetBuffer.getInt(2 * MapVector.OFFSET_WIDTH));
+ }
+ }
+
+ @Test
+ public void testMapWithMapKeyAndMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter keyWriter;
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Map<Integer, Integer>, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
Review comment:
Just like above, I added the data that will be in the resulting map as a comment. I also attempted to refactor some of the repetition into a function. Higher-level functions than writing a single entry (say, writing a map) seemed to cause too much complexity and make things difficult to read, since it requires passing around multiple writers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r582506810
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
Review comment:
I added a comment showing the resulting Map that the code is creating. I tried to pick a syntax that was both concise yet clear, however it gets challenging as the data type gets more complex.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r583064206
##########
File path: java/vector/src/main/codegen/templates/UnionFixedSizeListWriter.java
##########
@@ -169,6 +169,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
I tried digging into this a little bit more and stepped through what is done for list. I think? we are matching what is done for list, for example [see here](https://github.com/apache/arrow/blob/master/java/vector/src/main/codegen/templates/UnionListWriter.java#L162) where `list()` just returns the current `UnionListWriter`. Since we need `keysSorted` for map, there's an additional `map(boolean)` method, but just like `map()` we return the current writer. Appreciate any more insight you can provide here Bryan.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565618713
##########
File path: java/vector/src/main/codegen/templates/UnionWriter.java
##########
@@ -110,6 +143,26 @@ public ListWriter asList() {
return getListWriter();
}
+ private MapWriter getMapWriter() {
+ // returns existing writer
+ Preconditions.checkNotNull(mapWriter);
Review comment:
I think this is another case of, we need keysSorted in order to create the writer so we can't construct a new writer, but must rely on an existing one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-764632071
@nbruno Thanks for the PR. For the Java implementation, the Map type is implemented as a List of Struct. So we should already have supported the Map type? What is the motivation of the new implementation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565616074
##########
File path: java/vector/src/main/codegen/templates/UnionListWriter.java
##########
@@ -176,6 +176,29 @@ public StructWriter struct(String name) {
return structWriter;
}
+ @Override
+ public MapWriter map() {
+ return writer;
+ }
+
+ @Override
+ public MapWriter map(String name) {
+ MapWriter mapWriter = writer.map(name);
+ return mapWriter;
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ writer.map(keysSorted);
Review comment:
I responded above on why `mapWriter` isn't returned here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r582505891
##########
File path: java/vector/src/main/codegen/templates/UnionVector.java
##########
@@ -647,6 +673,8 @@ public ValueVector getVectorByType(int typeId, ArrowType arrowType) {
return getStruct();
case LIST:
return getList();
+ case MAP:
+ return getMap();
Review comment:
I believe this should've been `getMap(name, arrowType)`. I updated the PR with a unit test for `UnionVector`.
##########
File path: java/vector/src/test/java/org/apache/arrow/vector/TestMapVector.java
##########
@@ -624,6 +625,423 @@ public void testMapWithListValue() throws Exception {
}
}
+ @Test
+ public void testMapWithMapValue() throws Exception {
+ try (MapVector mapVector = MapVector.empty("sourceVector", allocator, false)) {
+
+ UnionMapWriter mapWriter = mapVector.getWriter();
+ MapWriter valueWriter;
+
+ // we are essentially writing Map<Long, Map<Long, Long>>
+
+ mapWriter.setPosition(0);
+ mapWriter.startMap();
+
+ mapWriter.startEntry();
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-826489371
Sorry will take a look tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-764632071
@nbruno Thanks for the PR. For the Java implementation, the Map type is implemented as a List of Struct. So we should already have supported the Map type? What is the motivation of the new implementation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r583484237
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,41 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ return map(name, false);
+ }
+
+ @Override
+ public MapWriter map(String name, boolean keysSorted) {
+ FieldWriter writer = fields.get(handleCase(name));
+ if(writer == null) {
+ ValueVector vector;
+ ValueVector currentVector = container.getChild(name);
+ MapVector v = container.addOrGet(name,
+ new FieldType(addVectorAsNullable,
+ new ArrowType.Map(keysSorted)
+ ,null, null),
+ MapVector.class);
+ writer = new PromotableWriter(v, container, getNullableStructWriterFactory());
+ vector = v;
+ if (currentVector == null || currentVector != vector) {
+ if(this.initialCapacity > 0) {
+ vector.setInitialCapacity(this.initialCapacity);
+ }
+ vector.allocateNewSafe();
+ }
+ writer.setPosition(idx());
+ fields.put(handleCase(name), writer);
+ } else {
+ if (writer instanceof PromotableWriter) {
Review comment:
no need for a nested `if` statement, just use `else if`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] BryanCutler commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
BryanCutler commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r570609184
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
Review comment:
We should probably have the writer default to `keysSorted=false` since that will be the most common usage and would only be used if someone really needed this flag. Would that allow you to change this to create a writer by default then, and only supply `keysSorted` optionally?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-786525254
Had another pass. Mostly look good to me. I am also a little concerned about @BryanCutler 's comment (https://github.com/apache/arrow/pull/9151/files#r570608317)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r582505384
##########
File path: java/vector/src/main/codegen/templates/UnionWriter.java
##########
@@ -110,6 +143,26 @@ public ListWriter asList() {
return getListWriter();
}
+ private MapWriter getMapWriter() {
+ // returns existing writer
+ Preconditions.checkNotNull(mapWriter);
Review comment:
I've updated the PR to assume `keysSorted = false` by default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r582504347
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
Review comment:
With the default of `keysSorted = false` we can create the writer by default. I've updated this PR with that change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-764664522
@liyafan82 It might be implicitly supported, but you'd have to be aware of that implementation detail in order to work with it. It's also less fluent to work with a list of structs to imply a map in the readers / writers, rather than a dedicated map API. It was also noted as a gap in the API on the [mailing list](https://lists.apache.org/thread.html/r6e2de4be2cdea11e3f277aff2c23b2bda0782d9460d4dddf5c11f240%40%3Cuser.arrow.apache.org%3E).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r565612067
##########
File path: java/vector/src/main/codegen/templates/StructWriters.java
##########
@@ -184,6 +189,44 @@ public ListWriter list(String name) {
return writer;
}
+ @Override
+ public MapWriter map(String name) {
+ // returns existing writer
+ final FieldWriter writer = fields.get(handleCase(name));
+ Preconditions.checkNotNull(writer);
Review comment:
This should be following the paradigm for other complex types that require arguments. In general, `foo(String name)` seems to be used to return an existing writer when the given type requires arguments. The existing writer is expected to have already been created by calling a same-named method with the type's arguments.
For a Map, you need to send in keysSorted for us to be able to construct a writer. There's other types that you need to specify some additional info in order to be able to construct it (Duration needs a TimeUnit, Decimal needs scale / precision). I am guessing that these methods are provided for convenience, so the caller can do:
writer.decimal256("example", 2, 5).writeDecimal256(..)
// Now, they can reference that vector later on without constructing a new writer:
writer.decimal256("example").writeDecimal256(..)
That's why this is returning an existing writer and failing otherwise.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-826489371
Sorry will take a look tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-757389014
https://issues.apache.org/jira/browse/ARROW-11173
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-800867128
@nbruno are you able to address the latest round of feedback?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-819063993
@emkornfield I replied to the latest review comments. If there is additional feedback please let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] liyafan82 commented on a change in pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#discussion_r583475049
##########
File path: java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
##########
@@ -144,6 +174,16 @@ public ListWriter list() {
return getWriter(MinorType.LIST).list();
}
+ @Override
+ public MapWriter map() {
+ return getWriter(MinorType.LIST).map();
+ }
+
+ @Override
+ public MapWriter map(boolean keysSorted) {
+ return getWriter(MinorType.MAP, new ArrowType.Map(keysSorted));
Review comment:
Do we need to have two static final instaces of `ArrowType.Map` that corresponds to cases when keysSorted equals to `true` and `false`?
This may avoid some object allocations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nbruno commented on pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
nbruno commented on pull request #9151:
URL: https://github.com/apache/arrow/pull/9151#issuecomment-764664522
@liyafan82 It might be implicitly supported, but you'd have to be aware of that implementation detail in order to work with it. It's also less fluent to work with a list of structs to imply a map in the readers / writers, rather than a dedicated map API. It was also noted as a gap in the API on the [mailing list](https://lists.apache.org/thread.html/r6e2de4be2cdea11e3f277aff2c23b2bda0782d9460d4dddf5c11f240%40%3Cuser.arrow.apache.org%3E).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield closed pull request #9151: ARROW-11173: [Java] Add map type in complex reader / writer
Posted by GitBox <gi...@apache.org>.
emkornfield closed pull request #9151:
URL: https://github.com/apache/arrow/pull/9151
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org