Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/909#discussion_r134061376
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
---
@@ -359,30 +361,109 @@ public Mutator(OperatorExecContext oContext,
BufferAllocator allocator, VectorCo
public <T extends ValueVector> T addField(MaterializedField field,
Class<T> clazz) throws
SchemaChangeException {
// Check if the field exists.
- ValueVector v = fieldVectorMap.get(field.getPath());
- if (v == null || v.getClass() != clazz) {
+ ValueVector vector = fieldVectorMap.get(field.getName());
+ ValueVector childVector = vector;
+ // if a vector does not exist yet, creates value vector, or if it
exists and has map type, omit this code
+ if (vector == null || (vector.getClass() != clazz
+ && (vector.getField().getType().getMinorType() != MinorType.MAP
+ || field.getType().getMinorType() != MinorType.MAP))) {
// Field does not exist--add it to the map and the output
container.
- v = TypeHelper.getNewVector(field, allocator, callBack);
- if (!clazz.isAssignableFrom(v.getClass())) {
+ vector = TypeHelper.getNewVector(field, allocator, callBack);
+ childVector = vector;
+ // gets inner field if the map was created the first time
+ if (field.getType().getMinorType() == MinorType.MAP) {
+ childVector = getChildVectorByField(vector, field);
+ } else if (!clazz.isAssignableFrom(vector.getClass())) {
throw new SchemaChangeException(
String.format(
"The class that was provided, %s, does not correspond to
the "
+ "expected vector type of %s.",
- clazz.getSimpleName(), v.getClass().getSimpleName()));
+ clazz.getSimpleName(),
vector.getClass().getSimpleName()));
}
- final ValueVector old = fieldVectorMap.put(field.getPath(), v);
+ final ValueVector old = fieldVectorMap.put(field.getName(),
vector);
if (old != null) {
old.clear();
container.remove(old);
}
- container.add(v);
+ container.add(vector);
// Added new vectors to the container--mark that the schema has
changed.
schemaChanged = true;
}
+ // otherwise, checks that field and existing vector have a map type
+ // and adds child fields from the field to the vector
+ else if (field.getType().getMinorType() == MinorType.MAP
+ && vector.getField().getType().getMinorType() ==
MinorType.MAP
+ && !field.getChildren().isEmpty()) {
+ // an incoming field contains only single child since it determines
+ // full name path of the field in the schema
+ childVector = addNestedChildToMap((MapVector) vector,
Iterables.getLast(field.getChildren()));
+ schemaChanged = true;
+ }
- return clazz.cast(v);
+ return clazz.cast(childVector);
+ }
+
+ /**
+ * Finds and returns value vector which path corresponds to the
specified field.
+ * If required vector is nested in the map, gets and returns this
vector from the map.
+ *
+ * @param valueVector vector that should be checked
+ * @param field field that corresponds to required vector
+ * @return value vector whose path corresponds to the specified field
+ *
+ * @throws SchemaChangeException if the field does not correspond to
the found vector
+ */
+ private ValueVector getChildVectorByField(ValueVector valueVector,
+ MaterializedField field)
throws SchemaChangeException {
+ if (field.getChildren().isEmpty()) {
+ if (valueVector.getField().equals(field)) {
+ return valueVector;
+ } else {
+ throw new SchemaChangeException(
+ String.format(
+ "The field that was provided, %s, does not correspond to the
"
+ + "expected vector type of %s.",
+ field, valueVector.getClass().getSimpleName()));
+ }
+ } else {
+ // an incoming field contains only single child since it determines
+ // full name path of the field in the schema
+ MaterializedField childField =
Iterables.getLast(field.getChildren());
+ return getChildVectorByField(((MapVector)
valueVector).getChild(childField.getName()), childField);
+ }
--- End diff --
Not sure I understand this. I understand the idea that if the vector is
given, we want to check if the vector schema equals the given field. (But, not
that, in general, the `isEquals()` won't work. A nullable vector will include
the `$bits$` field as a nested column. A Varchar will inlcude `$offsets$`. But,
the schema coming from the client won't contain these hidden fields. As a
result, the two fields won't compare equal. I recently introduced an
`isEquivalent()` method to handle these cases. Or, you can just check the
`MajorType`s. (Though, you have to decide if a Varchar of precision 10 is the
same as a Varchar of precision 20, say...)
Again, the new result set loader handles all this.
The part I don't follow is if the field has children. Why do we expect that
the new field must match the last of the children? And, do we really want to do
a sequential search through field names for every value of every column of
every row?
Yet again, the new result set loader fixes all this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
