asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <>
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:


Please format the code according to:

I found a number of files that do not follow the code format style (if my Eclipse works correctly...).
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/

Line 29: public class DeepEqualityVisitorUtils {
Race conditions --- because of the usage of shared static states.
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/

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.
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/visitors/

Line 35: class RecordDeepEqualityAccessor {

"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,;
the getInteger seems not needed.  Instead, you probably can use:

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

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),
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I3621ebdd71c7cd91b50d77a972ad863cea7fcbc2
Gerrit-PatchSet: 12
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Heri Ramampiaro <>
Gerrit-Reviewer: Heri Ramampiaro <>
Gerrit-Reviewer: Ian Maxon <>
Gerrit-Reviewer: Jenkins <>
Gerrit-Reviewer: Preston Carman <>
Gerrit-Reviewer: Steven Jacobs <>
Gerrit-Reviewer: Till Westmann <>
Gerrit-Reviewer: Yingyi Bu <>
Gerrit-HasComments: Yes

View raw message