asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Heri Ramampiaro (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: ASTERIXDB-1187 and ASTERIXDB-1162 fixes, as well as updates ...
Date Wed, 02 Dec 2015 14:50:02 GMT
Heri Ramampiaro has posted comments on this change.

Change subject: ASTERIXDB-1187 and ASTERIXDB-1162 fixes, as well as updates on the internal
functions
......................................................................


Patch Set 12:

(15 comments)

I have now submitted a new patch to address the issues raised by Yingyi. 
Best,
-heri

https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddFieldsTypeComputer.java
File asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddFieldsTypeComputer.java:

Line 69:             return BuiltinType.ANY;
> It seems to me that we should throw an exception here. Thoughts?
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java
File asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordMergeTypeComputer.java:

Line 112:                     }
> How about OrderedList and UnOrderedList, especially when they are nested an
I do support the ideas related nested lists. For simplicity the support of complex objects
other than records is generally taken care of at runtime. Our new deep equality check functionality
allows this. Please note that the lists have to be 100% equal for the fields to be merged.
In any case, I will investigate this further for the next version. For now the main focus
will be to address the issues related to open types.


https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRemoveFieldsTypeComputer.java
File asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordRemoveFieldsTypeComputer.java:

Line 62:     private final Deque<String> fieldPathStack = new ArrayDeque<>();
> same as other type computers:
Done


https://asterix-gerrit.ics.uci.edu/#/c/298/12/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/TypeComputerUtils.java
File asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/TypeComputerUtils.java:

Line 30:     private TypeComputerUtils() {
> It seems you don't need this constructor.
Done


Line 31:         
> white space.
Done


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

Line 58:     
> white space.
Done


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

Line 36:     private final Map<IVisitablePointable, ListDeepEqualityAccessor> laccessorToEqulity
=
> accessor-->pointable
Done


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 95:         }
> just call "item0.accept(visitor, arg);" ?
Done


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


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?
Done


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


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


Line 101:             if(fieldType0 != PointableUtils.getTypeTag(fieldTypes1.get(index1)))
{
> just call fieldTypes0.get(index0).accept(fieldTypes1.get(index1), ...),to l
Done


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


Line 119:             }
> Just call call 
Done


-- 
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