asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Internal functions for record manipulation, deep equality co...
Date Thu, 19 Nov 2015 21:21:21 GMT
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

Mime
View raw message