paul-rogers commented on a change in pull request #1383: DRILL-6613: Refactor
MaterializedField
URL: https://github.com/apache/drill/pull/1383#discussion_r204098796
##########
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/MaterializedField.java
##########
@@ -49,39 +54,79 @@ private MaterializedField(String name, MajorType type,
LinkedHashSet<Materialize
this.children = children;
}
+ private MaterializedField(String name, MajorType type, int size) {
+ this(name, type, new LinkedHashSet<>(size));
+ }
+
+ private <T> void copyFrom(Collection<T> source, Function<T,
MaterializedField> transformation) {
+ Preconditions.checkState(children.isEmpty());
+ source.forEach(child -> children.add(transformation.apply(child)));
+ }
+
+ public static MaterializedField create(String name, MajorType type) {
+ return new MaterializedField(name, type, 0);
+ }
+
public static MaterializedField create(SerializedField serField) {
- LinkedHashSet<MaterializedField> children = new LinkedHashSet<>();
- for (SerializedField sf : serField.getChildList()) {
- children.add(MaterializedField.create(sf));
+ MaterializedField field = new
MaterializedField(serField.getNamePart().getName(), serField.getMajorType(),
serField.getChildCount());
+ if (OFFSETS_FIELD.equals(field)) {
+ return OFFSETS_FIELD;
}
- return new MaterializedField(serField.getNamePart().getName(),
serField.getMajorType(), children);
+ field.copyFrom(serField.getChildList(), MaterializedField::create);
+ return field;
}
- /**
- * Create and return a serialized field based on the current state.
- */
- public SerializedField getSerializedField() {
- SerializedField.Builder serializedFieldBuilder = getAsBuilder();
- for(MaterializedField childMaterializedField : getChildren()) {
-
serializedFieldBuilder.addChild(childMaterializedField.getSerializedField());
+ public MaterializedField copy() {
+ return copy(getName(), getType());
+ }
+
+ public MaterializedField copy(MajorType type) {
+ return copy(name, type);
+ }
+
+ public MaterializedField copy(String name) {
+ return copy(name, getType());
+ }
+
+ public MaterializedField copy(String name, final MajorType type) {
+ if (this == OFFSETS_FIELD) {
Review comment:
Actually, the semantics here are wrong. This code assumes that it is OK to
reuse the one particular field. As noted above, this class should not be making
such an assertion.
Your fix does suggest a more general solution, one that can be done here.
There is a large class of `MaterializedField`s that can be reused rather than
copying (in the context of the `PartionerTemplate` only): those with no
children.
Of all the vector types, only required, fixed-width scalar (non-map,
non-union, non-list, non-varchar) vectors will never carry children.
Nullable vectors carry children for bits. Arrays and Varchars carry offset
vectors. Maps, unions and lists have children.
So, you can generalize this code (and rename it, because it is not a true
copy), to "copySafe" and return a true copy if the type carries children
(subtypes or child fields), otherwise, return the `MaterializedField` itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
