Mailing-List: contact notifications-help@asterixdb.incubator.apache.org;
 run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.incubator.apache.org
Date: Thu, 19 Nov 2015 13:21:21 -0800
From: "Yingyi Bu (Code Review)" <do-not-reply@asterixdb.incubator.apache.org>
To: Heri Ramampiaro <heriram@gmail.com>
CC: Steven Jacobs <sjaco002@ucr.edu>,
	Preston Carman <prestonc@apache.org>,
	Ian Maxon <imaxon@apache.org>,
	Jenkins <jenkins@fulliautomatix.ics.uci.edu>,
	Till Westmann <tillw@apache.org>
Reply-To: buyingyi@gmail.com
Subject: Change in asterixdb[master]: Internal functions for record
 manipulation, deep equality co...
In-Reply-To: 
 <gerrit.1435689999508.I3621ebdd71c7cd91b50d77a972ad863cea7fcbc2@asterix-gerrit.ics.uci.edu>
References: 
 <gerrit.1435689999508.I3621ebdd71c7cd91b50d77a972ad863cea7fcbc2@asterix-gerrit.ics.uci.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.8.4
Message-Id: <20151119212121.B31C7240D73@unhygienix.ics.uci.edu>

Yingyi Bu has posted comments on this change.

Change subject: Internal functions for record manipulation, deep equality comparison and fix for ASTERIXDB-1162
......................................................................


Patch Set 12:

(11 comments)

Please format the code according to:
https://asterixdb.incubator.apache.org/dev-setup.html

I found a number of files that do not follow the code format style (if my Eclipse works correctly...).

https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/DeepEqualityVisitorUtils.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/DeepEqualityVisitorUtils.java:

Line 29: public class DeepEqualityVisitorUtils {
Race conditions --- because of the usage of shared static states.


https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/ListDeepEqualityAccessor.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/ListDeepEqualityAccessor.java:

Line 81:         Pair<IVisitablePointable, Boolean> arg = new Pair<IVisitablePointable, Boolean>(item1, false);
avoid per-tuple object creation overhead.


Line 95:         }
just call "item0.accept(visitor, arg);" ?


Line 105:             ATypeTag fieldType0 = PointableUtils.getTypeTag(itemTagTypes0.get(i));
just call itemTagTypes0.get(i).accept(itemTagTypes1.get(i))

The visitor pattern allows you to let a "compare" implementation just focuses on the current level, and call visitor.accept(...) for the next level.


https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/RecordDeepEqualityAccessor.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/RecordDeepEqualityAccessor.java:

Line 35: class RecordDeepEqualityAccessor {
Accessor->Checker?

"Accessor" seems a bit misleading.


Line 69:         if (s0 != s1)
wrap if-else branches with {}.  That makes code more readable.


Line 77:             BinaryHashMap.BinaryEntry entry = hashMap.put(keyEntry, valEntry);
entry is not used so remove it.


Line 99:             int index0 = IntegerPointable.getInteger(entry.buf, entry.off);
the getInteger seems not needed.  Instead, you probably can use:
AInt32SerializerDeserializer.getInt()


Line 101:             if(fieldType0 != PointableUtils.getTypeTag(fieldTypes1.get(index1))) {
just call fieldTypes0.get(index0).accept(fieldTypes1.get(index1), ...),to leverage the visitor pattern?

BTW, variables could be named in a better way, e.g., 0-->"Left", 1-->"Right".


Line 105:             arg = new Pair<IVisitablePointable, Boolean>(fieldValues1.get(index1), false);
avoid per-tuple object creation.

ARecordCaster could be a sample to look at.


Line 119:             }
Just call call 
"fieldValues0.get(index0).accept(visitor, arg);"?
the visitor will do double-dispatch and make the code simpler.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3621ebdd71c7cd91b50d77a972ad863cea7fcbc2
Gerrit-PatchSet: 12
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Heri Ramampiaro <heriram@gmail.com>
Gerrit-Reviewer: Heri Ramampiaro <heriram@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Preston Carman <prestonc@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes