paul-rogers commented on a change in pull request #1383: DRILL-6613: Refactor
MaterializedField
URL: https://github.com/apache/drill/pull/1383#discussion_r204165621
##########
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:
Yes, I mean that other instances can be reused. By "wrong semantics", I mean
that we have singled out one particular field instance to be "atomic" and
reusable. Nothing wrong with that. It is simply awkward:
* Not only is the offsets field reusable, so is the bits field. So, we' see
to be making a statement that there is something special about offsets.
* Offsets is reusable because it is fixed size, required. So, if we handle
the case for one fixed size, required field, we should handle all of them.
Else, again, we're saying that something is special about offsets.
* Further, since the resulting code copies sometimes, but not others, this
is not a true "copy" operation. It is a "copy if necessary" operation.
Implementation-wise, the "copy if necessary" should either return the same
field (if no copy is needed) or call a "copy" method to make an actual copy
when needed.
None of this is wrong; the code here works. But, if we are going to the
trouble to improve working code, we might want to handle the other cases as
well.
----------------------------------------------------------------
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
