vrozov commented on a change in pull request #1383: DRILL-6613: Refactor
MaterializedField
URL: https://github.com/apache/drill/pull/1383#discussion_r205980354
##########
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:
> 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.
Offset field was already handled differently compared to other fields
including bits field prior to the PR. The only new behavior that the PR
introduces is a singleton pattern for offset field during copy operation to
avoid compare by name where compare by identity should be used in the first
place. As there was no compare by name for bits, bits field is not handled. I
do agree that bits can be handled similar to offset field, but it is not a goal
for this PR. The goal of the PR outlined in the JIRA description is to avoid
using `clone()` for `MaterializedField`.
> 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.
Offset field is reusable because it is immutable (name, type and children
are all final). Bits field can be also made reusable, but it is not a goal of
the PR. The PR does not make offset field special as it was already handled
differently prior to the PR.
>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.
It is the only `copy` operation that is available outside of
`MaterializedField` class and no any other `copy` should exist. The `copy`
decides whether or not it needs to create another instance or it can return a
singleton.
----------------------------------------------------------------
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
