You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@asterixdb.apache.org by "Wail Alkowaileet (Code Review)" <do...@asterixdb.incubator.apache.org> on 2016/10/07 06:28:17 UTC

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Wail Alkowaileet has uploaded a new change for review.

  https://asterix-gerrit.ics.uci.edu/1260

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................

ASTERIXDB-1673 and some small cleanups

- Fix potential object creation issue in ARecordCaster.
- refactor getFieldOffsetById() in ARecordSerializerDeserailzer.
- remove dead code in method set() in ARecordVisitablePointable.
- Introduce unrestable allocation in PointableAllocator as static method.

Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
---
M asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
6 files changed, 74 insertions(+), 95 deletions(-)


  git pull ssh://asterix-gerrit.ics.uci.edu:29418/asterixdb refs/changes/60/1260/1

diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
index 55c9d13..fb5d0c7 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
@@ -214,8 +214,7 @@
 
     @SuppressWarnings("unchecked")
     public static void serializeSimpleSchemalessRecord(List<Pair<String, String>> record, DataOutput dataOutput,
-            boolean writeTypeTag)
-            throws HyracksDataException {
+            boolean writeTypeTag) throws HyracksDataException {
         ISerializerDeserializer<AString> stringSerde = AqlSerializerDeserializerProvider.INSTANCE
                 .getSerializerDeserializer(BuiltinType.ASTRING);
         RecordBuilder confRecordBuilder = new RecordBuilder();
@@ -268,67 +267,39 @@
 
     public static final int getFieldOffsetById(byte[] serRecord, int offset, int fieldId, int nullBitmapSize,
             boolean isOpen) {
-        byte nullTestCode = (byte) (1 << (7 - 2 * (fieldId % 4)));
-        byte missingTestCode = (byte) (1 << (7 - 2 * (fieldId % 4) - 1));
+        final byte nullTestCode = (byte) (1 << (7 - 2 * (fieldId % 4)));
+        final byte missingTestCode = (byte) (1 << (7 - 2 * (fieldId % 4) - 1));
+
+        //early exit if not Record
+        if (serRecord[offset] != ATypeTag.SERIALIZED_RECORD_TYPE_TAG) {
+            return -1;
+        }
+
+        //advance to isExpanded or numberOfSchemaFields
+        int pointer = offset + 5;
+
         if (isOpen) {
-            if (serRecord[0 + offset] == ATypeTag.RECORD.serialize()) {
-                // 5 is the index of the byte that determines whether the record
-                // is expanded or not, i.e. it has an open part.
-                if (serRecord[5 + offset] == 1) { // true
-                    if (nullBitmapSize > 0) {
-                        // 14 = tag (1) + record Size (4) + isExpanded (1) +
-                        // offset of openPart (4) + number of closed fields (4)
-                        int pos = 14 + offset + fieldId / 4;
-                        if ((serRecord[pos] & nullTestCode) == 0) {
-                            // the field value is null
-                            return 0;
-                        }
-                        if ((serRecord[pos] & missingTestCode) == 0) {
-                            // the field value is missing
-                            return -1;
-                        }
-                    }
-                    return offset + AInt32SerializerDeserializer.getInt(serRecord,
-                            14 + offset + nullBitmapSize + (4 * fieldId));
-                } else {
-                    if (nullBitmapSize > 0) {
-                        // 9 = tag (1) + record Size (4) + isExpanded (1) +
-                        // number of closed fields (4)
-                        int pos = 10 + offset + fieldId / 4;
-                        if ((serRecord[pos] & nullTestCode) == 0) {
-                            // the field value is null
-                            return 0;
-                        }
-                        if ((serRecord[pos] & missingTestCode) == 0) {
-                            // the field value is missing
-                            return -1;
-                        }
-                    }
-                    return offset + AInt32SerializerDeserializer.getInt(serRecord,
-                            10 + offset + nullBitmapSize + (4 * fieldId));
-                }
-            } else {
+            final boolean isExpanded = serRecord[pointer] == 1;
+            //if isExpanded, advance to numberOfSchemaFields
+            pointer += 1 + (isExpanded ? 4 : 0);
+        }
+
+        //advance to nullBitmap
+        pointer += 4;
+
+        if (nullBitmapSize > 0) {
+            final int pos = pointer + fieldId / 4;
+            if ((serRecord[pos] & nullTestCode) == 0) {
+                // the field value is null
+                return 0;
+            }
+            if ((serRecord[pos] & missingTestCode) == 0) {
+                // the field value is missing
                 return -1;
             }
-        } else {
-            if (serRecord[offset] != ATypeTag.SERIALIZED_RECORD_TYPE_TAG) {
-                return Integer.MIN_VALUE;
-            }
-            if (nullBitmapSize > 0) {
-                // 9 = tag (1) + record Size (4) + number of closed fields
-                // (4)
-                int pos = 9 + offset + fieldId / 4;
-                if ((serRecord[pos] & nullTestCode) == 0) {
-                    // the field value is null
-                    return 0;
-                }
-                if ((serRecord[pos] & missingTestCode) == 0) {
-                    // the field value is missing
-                    return -1;
-                }
-            }
-            return offset + AInt32SerializerDeserializer.getInt(serRecord, 9 + offset + nullBitmapSize + (4 * fieldId));
         }
+
+        return offset + AInt32SerializerDeserializer.getInt(serRecord, pointer + nullBitmapSize + (4 * fieldId));
     }
 
     public static final int getFieldOffsetByName(byte[] serRecord, int start, int len, byte[] fieldName, int nstart)
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
index 68fff2f..d3c18a9 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
@@ -75,17 +75,17 @@
 
     private final int numberOfSchemaFields;
     private final int[] fieldOffsets;
-    private final IVisitablePointable nullReference = AFlatValuePointable.FACTORY.create(null);
-    private final IVisitablePointable missingReference = AFlatValuePointable.FACTORY.create(null);
+    private final IVisitablePointable nullReference = PointableAllocator.allocateUnrestableEmpty();
+    private final IVisitablePointable missingReference = PointableAllocator.allocateUnrestableEmpty();
 
     private int closedPartTypeInfoSize = 0;
-    private int offsetArrayOffset;
     private ATypeTag typeTag;
 
     /**
      * private constructor, to prevent constructing it arbitrarily
      *
      * @param inputType
+     *            inputType should not be null. Use FULLY_OPEN_RECORD_TYPE instead.
      */
     public ARecordVisitablePointable(ARecordType inputType) {
         this.inputRecType = inputType;
@@ -165,27 +165,26 @@
 
         boolean isExpanded = false;
         int openPartOffset = 0;
-        int s = start;
-        int recordOffset = s;
-        if (inputRecType == null) {
-            openPartOffset = s + AInt32SerializerDeserializer.getInt(b, s + 6);
-            s += 8;
-            isExpanded = true;
-        } else {
-            if (inputRecType.isOpen()) {
-                isExpanded = b[s + 5] == 1 ? true : false;
-                if (isExpanded) {
-                    openPartOffset = s + AInt32SerializerDeserializer.getInt(b, s + 6);
-                    s += 10;
-                } else {
-                    s += 6;
-                }
-            } else {
-                s += 5;
+        int recordOffset = start;
+        int offsetArrayOffset;
+
+        //advance to either isExpanded or numberOfSchemaFields
+        int s = start + 5;
+        //inputRecType will never be null.
+        if (inputRecType.isOpen()) {
+            isExpanded = b[s] == 1;
+            //advance either to openPartOffset or numberOfSchemaFields
+            s += 1;
+            if (isExpanded) {
+                openPartOffset = start + AInt32SerializerDeserializer.getInt(b, s);
+                //advance to numberOfSchemaFields
+                s += 4;
             }
         }
+
         try {
             if (numberOfSchemaFields > 0) {
+                //advance to nullBitMap if hasOptionalFields, or fieldOffsets
                 s += 4;
                 int nullBitMapOffset = 0;
                 boolean hasOptionalFields = NonTaggedFormatUtil.hasOptionalField(inputRecType);
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
index 0f66a57..c88b95c 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
@@ -63,6 +63,16 @@
     }
 
     /**
+     * This method should ONLY be used for long lasting IVisitablePointable.
+     *
+     * @return
+     *         a generic type IVisitablePointable.
+     */
+    public static IVisitablePointable allocateUnrestableEmpty() {
+        return AFlatValuePointable.FACTORY.create(null);
+    }
+
+    /**
      * allocate closed part value pointable
      *
      * @param type
@@ -98,8 +108,8 @@
                 if (listItemType.isDerivedType())
                     return allocateFieldValue(listItemType, b, offset + 1);
                 else
-                    return listValueAllocator.allocate(unorederedListTypeAllocator.allocate(TypeTagUtil
-                            .getBuiltinTypeByTag(listItemType)));
+                    return listValueAllocator.allocate(
+                            unorederedListTypeAllocator.allocate(TypeTagUtil.getBuiltinTypeByTag(listItemType)));
             }
         } else if (typeTag.equals(ATypeTag.ORDEREDLIST)) {
             ATypeTag listItemType = EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(b[offset]);
@@ -109,8 +119,8 @@
                 if (listItemType.isDerivedType())
                     return allocateFieldValue(listItemType, b, offset + 1);
                 else
-                    return listValueAllocator.allocate(orederedListTypeAllocator.allocate(TypeTagUtil
-                            .getBuiltinTypeByTag(listItemType)));
+                    return listValueAllocator.allocate(
+                            orederedListTypeAllocator.allocate(TypeTagUtil.getBuiltinTypeByTag(listItemType)));
             }
         } else
             return flatValueAllocator.allocate(null);
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
index 00c4075..e57fa06 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
@@ -110,8 +110,8 @@
             arg.first.set(accessor);
             return null;
         }
-        ATypeTag inputTypeTag =
-                EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(accessor.getByteArray()[accessor.getStartOffset()]);
+        ATypeTag inputTypeTag = EnumDeserializer.ATYPETAGDESERIALIZER
+                .deserialize(accessor.getByteArray()[accessor.getStartOffset()]);
         if (!needPromote(inputTypeTag, reqTypeTag)) {
             arg.first.set(accessor);
         } else {
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
index 5dee990..7115827 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
@@ -43,11 +43,9 @@
  * ACastVisitor.
  */
 class AListCaster {
-    // pointable allocator
-    private final PointableAllocator allocator = new PointableAllocator();
 
     // for storing the cast result
-    private final IVisitablePointable itemTempReference = allocator.allocateEmpty();
+    private final IVisitablePointable itemTempReference = PointableAllocator.allocateUnrestableEmpty();
     private final Triple<IVisitablePointable, IAType, Boolean> itemVisitorArg = new Triple<>(itemTempReference, null,
             null);
 
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
index 0293fcf..7e1fe46 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
@@ -71,18 +71,18 @@
     private final DataOutputStream dos = new DataOutputStream(bos);
 
     private final RecordBuilder recBuilder = new RecordBuilder();
-    private final IVisitablePointable nullTypeTag = allocator.allocateEmpty();
-    private final IVisitablePointable missingTypeTag = allocator.allocateEmpty();
+    private final IVisitablePointable nullTypeTag = PointableAllocator.allocateUnrestableEmpty();
+    private final IVisitablePointable missingTypeTag = PointableAllocator.allocateUnrestableEmpty();
 
-    private final IBinaryComparator fieldNameComparator =
-            PointableBinaryComparatorFactory.of(UTF8StringPointable.FACTORY).createBinaryComparator();
+    private final IBinaryComparator fieldNameComparator = PointableBinaryComparatorFactory
+            .of(UTF8StringPointable.FACTORY).createBinaryComparator();
 
     private final ByteArrayAccessibleOutputStream outputBos = new ByteArrayAccessibleOutputStream();
     private final DataOutputStream outputDos = new DataOutputStream(outputBos);
 
-    private final IVisitablePointable fieldTempReference = allocator.allocateEmpty();
-    private final Triple<IVisitablePointable, IAType, Boolean> nestedVisitorArg =
-            new Triple<>(fieldTempReference, null, null);
+    private final IVisitablePointable fieldTempReference = PointableAllocator.allocateUnrestableEmpty();
+    private final Triple<IVisitablePointable, IAType, Boolean> nestedVisitorArg = new Triple<>(fieldTempReference, null,
+            null);
 
     private int numInputFields = 0;
 
@@ -155,6 +155,7 @@
     private void loadRequiredType(ARecordType reqType) throws IOException {
         reqFieldNames.clear();
         reqFieldTypeTags.clear();
+        allocator.reset();
 
         cachedReqType = reqType;
         int numSchemaFields = reqType.getFieldTypes().length;

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Posted by "Jenkins (Code Review)" <do...@asterixdb.incubator.apache.org>.
Jenkins has posted comments on this change.

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................


Patch Set 1:

Build Started https://asterix-jenkins.ics.uci.edu/job/asterix-gerrit-notopic/2951/

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-HasComments: No

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Posted by "Jenkins (Code Review)" <do...@asterixdb.incubator.apache.org>.
Jenkins has posted comments on this change.

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................


Patch Set 1: Integration-Tests+1

Integration Tests Successful

https://asterix-jenkins.ics.uci.edu/job/asterix-gerrit-integration-tests/869/ : SUCCESS

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-HasComments: No

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Posted by "Jenkins (Code Review)" <do...@asterixdb.incubator.apache.org>.
Jenkins has posted comments on this change.

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................


Patch Set 1:

Integration Tests Started https://asterix-jenkins.ics.uci.edu/job/asterix-gerrit-integration-tests/869/

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-HasComments: No

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Posted by "Yingyi Bu (Code Review)" <do...@asterixdb.incubator.apache.org>.
Yingyi Bu has submitted this change and it was merged.

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................


ASTERIXDB-1673 and some small cleanups

- Fix potential object creation issue in ARecordCaster.
- refactor getFieldOffsetById() in ARecordSerializerDeserailzer.
- remove dead code in method set() in ARecordVisitablePointable.
- Introduce unrestable allocation in PointableAllocator as static method.

Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Reviewed-on: https://asterix-gerrit.ics.uci.edu/1260
Sonar-Qube: Jenkins <je...@fulliautomatix.ics.uci.edu>
Tested-by: Jenkins <je...@fulliautomatix.ics.uci.edu>
Integration-Tests: Jenkins <je...@fulliautomatix.ics.uci.edu>
Reviewed-by: Yingyi Bu <bu...@gmail.com>
---
M asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
M asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
6 files changed, 74 insertions(+), 95 deletions(-)

Approvals:
  Yingyi Bu: Looks good to me, approved
  Jenkins: Verified; No violations found; Verified



diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
index 55c9d13..fb5d0c7 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/serde/ARecordSerializerDeserializer.java
@@ -214,8 +214,7 @@
 
     @SuppressWarnings("unchecked")
     public static void serializeSimpleSchemalessRecord(List<Pair<String, String>> record, DataOutput dataOutput,
-            boolean writeTypeTag)
-            throws HyracksDataException {
+            boolean writeTypeTag) throws HyracksDataException {
         ISerializerDeserializer<AString> stringSerde = AqlSerializerDeserializerProvider.INSTANCE
                 .getSerializerDeserializer(BuiltinType.ASTRING);
         RecordBuilder confRecordBuilder = new RecordBuilder();
@@ -268,67 +267,39 @@
 
     public static final int getFieldOffsetById(byte[] serRecord, int offset, int fieldId, int nullBitmapSize,
             boolean isOpen) {
-        byte nullTestCode = (byte) (1 << (7 - 2 * (fieldId % 4)));
-        byte missingTestCode = (byte) (1 << (7 - 2 * (fieldId % 4) - 1));
+        final byte nullTestCode = (byte) (1 << (7 - 2 * (fieldId % 4)));
+        final byte missingTestCode = (byte) (1 << (7 - 2 * (fieldId % 4) - 1));
+
+        //early exit if not Record
+        if (serRecord[offset] != ATypeTag.SERIALIZED_RECORD_TYPE_TAG) {
+            return -1;
+        }
+
+        //advance to isExpanded or numberOfSchemaFields
+        int pointer = offset + 5;
+
         if (isOpen) {
-            if (serRecord[0 + offset] == ATypeTag.RECORD.serialize()) {
-                // 5 is the index of the byte that determines whether the record
-                // is expanded or not, i.e. it has an open part.
-                if (serRecord[5 + offset] == 1) { // true
-                    if (nullBitmapSize > 0) {
-                        // 14 = tag (1) + record Size (4) + isExpanded (1) +
-                        // offset of openPart (4) + number of closed fields (4)
-                        int pos = 14 + offset + fieldId / 4;
-                        if ((serRecord[pos] & nullTestCode) == 0) {
-                            // the field value is null
-                            return 0;
-                        }
-                        if ((serRecord[pos] & missingTestCode) == 0) {
-                            // the field value is missing
-                            return -1;
-                        }
-                    }
-                    return offset + AInt32SerializerDeserializer.getInt(serRecord,
-                            14 + offset + nullBitmapSize + (4 * fieldId));
-                } else {
-                    if (nullBitmapSize > 0) {
-                        // 9 = tag (1) + record Size (4) + isExpanded (1) +
-                        // number of closed fields (4)
-                        int pos = 10 + offset + fieldId / 4;
-                        if ((serRecord[pos] & nullTestCode) == 0) {
-                            // the field value is null
-                            return 0;
-                        }
-                        if ((serRecord[pos] & missingTestCode) == 0) {
-                            // the field value is missing
-                            return -1;
-                        }
-                    }
-                    return offset + AInt32SerializerDeserializer.getInt(serRecord,
-                            10 + offset + nullBitmapSize + (4 * fieldId));
-                }
-            } else {
+            final boolean isExpanded = serRecord[pointer] == 1;
+            //if isExpanded, advance to numberOfSchemaFields
+            pointer += 1 + (isExpanded ? 4 : 0);
+        }
+
+        //advance to nullBitmap
+        pointer += 4;
+
+        if (nullBitmapSize > 0) {
+            final int pos = pointer + fieldId / 4;
+            if ((serRecord[pos] & nullTestCode) == 0) {
+                // the field value is null
+                return 0;
+            }
+            if ((serRecord[pos] & missingTestCode) == 0) {
+                // the field value is missing
                 return -1;
             }
-        } else {
-            if (serRecord[offset] != ATypeTag.SERIALIZED_RECORD_TYPE_TAG) {
-                return Integer.MIN_VALUE;
-            }
-            if (nullBitmapSize > 0) {
-                // 9 = tag (1) + record Size (4) + number of closed fields
-                // (4)
-                int pos = 9 + offset + fieldId / 4;
-                if ((serRecord[pos] & nullTestCode) == 0) {
-                    // the field value is null
-                    return 0;
-                }
-                if ((serRecord[pos] & missingTestCode) == 0) {
-                    // the field value is missing
-                    return -1;
-                }
-            }
-            return offset + AInt32SerializerDeserializer.getInt(serRecord, 9 + offset + nullBitmapSize + (4 * fieldId));
         }
+
+        return offset + AInt32SerializerDeserializer.getInt(serRecord, pointer + nullBitmapSize + (4 * fieldId));
     }
 
     public static final int getFieldOffsetByName(byte[] serRecord, int start, int len, byte[] fieldName, int nstart)
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
index 68fff2f..d3c18a9 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/ARecordVisitablePointable.java
@@ -75,17 +75,17 @@
 
     private final int numberOfSchemaFields;
     private final int[] fieldOffsets;
-    private final IVisitablePointable nullReference = AFlatValuePointable.FACTORY.create(null);
-    private final IVisitablePointable missingReference = AFlatValuePointable.FACTORY.create(null);
+    private final IVisitablePointable nullReference = PointableAllocator.allocateUnrestableEmpty();
+    private final IVisitablePointable missingReference = PointableAllocator.allocateUnrestableEmpty();
 
     private int closedPartTypeInfoSize = 0;
-    private int offsetArrayOffset;
     private ATypeTag typeTag;
 
     /**
      * private constructor, to prevent constructing it arbitrarily
      *
      * @param inputType
+     *            inputType should not be null. Use FULLY_OPEN_RECORD_TYPE instead.
      */
     public ARecordVisitablePointable(ARecordType inputType) {
         this.inputRecType = inputType;
@@ -165,27 +165,26 @@
 
         boolean isExpanded = false;
         int openPartOffset = 0;
-        int s = start;
-        int recordOffset = s;
-        if (inputRecType == null) {
-            openPartOffset = s + AInt32SerializerDeserializer.getInt(b, s + 6);
-            s += 8;
-            isExpanded = true;
-        } else {
-            if (inputRecType.isOpen()) {
-                isExpanded = b[s + 5] == 1 ? true : false;
-                if (isExpanded) {
-                    openPartOffset = s + AInt32SerializerDeserializer.getInt(b, s + 6);
-                    s += 10;
-                } else {
-                    s += 6;
-                }
-            } else {
-                s += 5;
+        int recordOffset = start;
+        int offsetArrayOffset;
+
+        //advance to either isExpanded or numberOfSchemaFields
+        int s = start + 5;
+        //inputRecType will never be null.
+        if (inputRecType.isOpen()) {
+            isExpanded = b[s] == 1;
+            //advance either to openPartOffset or numberOfSchemaFields
+            s += 1;
+            if (isExpanded) {
+                openPartOffset = start + AInt32SerializerDeserializer.getInt(b, s);
+                //advance to numberOfSchemaFields
+                s += 4;
             }
         }
+
         try {
             if (numberOfSchemaFields > 0) {
+                //advance to nullBitMap if hasOptionalFields, or fieldOffsets
                 s += 4;
                 int nullBitMapOffset = 0;
                 boolean hasOptionalFields = NonTaggedFormatUtil.hasOptionalField(inputRecType);
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
index 0f66a57..c88b95c 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/PointableAllocator.java
@@ -63,6 +63,16 @@
     }
 
     /**
+     * This method should ONLY be used for long lasting IVisitablePointable.
+     *
+     * @return
+     *         a generic type IVisitablePointable.
+     */
+    public static IVisitablePointable allocateUnrestableEmpty() {
+        return AFlatValuePointable.FACTORY.create(null);
+    }
+
+    /**
      * allocate closed part value pointable
      *
      * @param type
@@ -98,8 +108,8 @@
                 if (listItemType.isDerivedType())
                     return allocateFieldValue(listItemType, b, offset + 1);
                 else
-                    return listValueAllocator.allocate(unorederedListTypeAllocator.allocate(TypeTagUtil
-                            .getBuiltinTypeByTag(listItemType)));
+                    return listValueAllocator.allocate(
+                            unorederedListTypeAllocator.allocate(TypeTagUtil.getBuiltinTypeByTag(listItemType)));
             }
         } else if (typeTag.equals(ATypeTag.ORDEREDLIST)) {
             ATypeTag listItemType = EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(b[offset]);
@@ -109,8 +119,8 @@
                 if (listItemType.isDerivedType())
                     return allocateFieldValue(listItemType, b, offset + 1);
                 else
-                    return listValueAllocator.allocate(orederedListTypeAllocator.allocate(TypeTagUtil
-                            .getBuiltinTypeByTag(listItemType)));
+                    return listValueAllocator.allocate(
+                            orederedListTypeAllocator.allocate(TypeTagUtil.getBuiltinTypeByTag(listItemType)));
             }
         } else
             return flatValueAllocator.allocate(null);
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
index 00c4075..e57fa06 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ACastVisitor.java
@@ -110,8 +110,8 @@
             arg.first.set(accessor);
             return null;
         }
-        ATypeTag inputTypeTag =
-                EnumDeserializer.ATYPETAGDESERIALIZER.deserialize(accessor.getByteArray()[accessor.getStartOffset()]);
+        ATypeTag inputTypeTag = EnumDeserializer.ATYPETAGDESERIALIZER
+                .deserialize(accessor.getByteArray()[accessor.getStartOffset()]);
         if (!needPromote(inputTypeTag, reqTypeTag)) {
             arg.first.set(accessor);
         } else {
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
index 5dee990..7115827 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/AListCaster.java
@@ -43,11 +43,9 @@
  * ACastVisitor.
  */
 class AListCaster {
-    // pointable allocator
-    private final PointableAllocator allocator = new PointableAllocator();
 
     // for storing the cast result
-    private final IVisitablePointable itemTempReference = allocator.allocateEmpty();
+    private final IVisitablePointable itemTempReference = PointableAllocator.allocateUnrestableEmpty();
     private final Triple<IVisitablePointable, IAType, Boolean> itemVisitorArg = new Triple<>(itemTempReference, null,
             null);
 
diff --git a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
index 0293fcf..7e1fe46 100644
--- a/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
+++ b/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/cast/ARecordCaster.java
@@ -71,18 +71,18 @@
     private final DataOutputStream dos = new DataOutputStream(bos);
 
     private final RecordBuilder recBuilder = new RecordBuilder();
-    private final IVisitablePointable nullTypeTag = allocator.allocateEmpty();
-    private final IVisitablePointable missingTypeTag = allocator.allocateEmpty();
+    private final IVisitablePointable nullTypeTag = PointableAllocator.allocateUnrestableEmpty();
+    private final IVisitablePointable missingTypeTag = PointableAllocator.allocateUnrestableEmpty();
 
-    private final IBinaryComparator fieldNameComparator =
-            PointableBinaryComparatorFactory.of(UTF8StringPointable.FACTORY).createBinaryComparator();
+    private final IBinaryComparator fieldNameComparator = PointableBinaryComparatorFactory
+            .of(UTF8StringPointable.FACTORY).createBinaryComparator();
 
     private final ByteArrayAccessibleOutputStream outputBos = new ByteArrayAccessibleOutputStream();
     private final DataOutputStream outputDos = new DataOutputStream(outputBos);
 
-    private final IVisitablePointable fieldTempReference = allocator.allocateEmpty();
-    private final Triple<IVisitablePointable, IAType, Boolean> nestedVisitorArg =
-            new Triple<>(fieldTempReference, null, null);
+    private final IVisitablePointable fieldTempReference = PointableAllocator.allocateUnrestableEmpty();
+    private final Triple<IVisitablePointable, IAType, Boolean> nestedVisitorArg = new Triple<>(fieldTempReference, null,
+            null);
 
     private int numInputFields = 0;
 
@@ -155,6 +155,7 @@
     private void loadRequiredType(ARecordType reqType) throws IOException {
         reqFieldNames.clear();
         reqFieldTypeTags.clear();
+        allocator.reset();
 
         cachedReqType = reqType;
         int numSchemaFields = reqType.getFieldTypes().length;

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 2
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Posted by "Wail Alkowaileet (Code Review)" <do...@asterixdb.incubator.apache.org>.
Wail Alkowaileet has posted comments on this change.

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................


Patch Set 1:

I found that we use PointableAllocator as a factory most of the time.
I didn't generalize the unrestable allocator methods to other types (i.e record and list). I want to see if that make sense first.

Many classes use PointableAllocator without calling reset. I think if the method contains the word unrestable, that will make newcomers aware that this might encounter object creation problem. Additionally, caller can allocate IVisitablePointable without creating the object PointableAllocator with its all overhead.

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-HasComments: No

Change in asterixdb[master]: ASTERIXDB-1673 and some small cleanups

Posted by "Yingyi Bu (Code Review)" <do...@asterixdb.incubator.apache.org>.
Yingyi Bu has posted comments on this change.

Change subject: ASTERIXDB-1673 and some small cleanups
......................................................................


Patch Set 1: Code-Review+2

-- 
To view, visit https://asterix-gerrit.ics.uci.edu/1260
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3802a5a33b46b58967ffb4a28a70ebe9671bfabf
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Wail Alkowaileet <wa...@gmail.com>
Gerrit-Reviewer: Yingyi Bu <bu...@gmail.com>
Gerrit-HasComments: No