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 20:02:42 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:

(9 comments)

>> (Maybe we should have submitted an open issue for this one?)
Sure, please open an issue!

I just added more comments.  There might be a few more -- it takes me a while to digest the
change.

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

Line 29: public class PointableTypeVisitor  implements IVisitablePointableVisitor<Void,
Pair<Boolean, Void>> {
PointableTypeVisitor -> CheckCompletelyOpenRecordTypeVisitor

Get the name closer to what it does


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

Line 53:     private final Deque<List<String>> stringListPool = new ArrayDeque<>();
A type computer should be stateless, because the computer uses xxxTypeComputer.INSTANCE.computeType(....).
  The type computer instance is shared across the entire JVM and hence shared across all queries
(potentially concurrent).

Creating objects in the compiler is fine because they anyway won't be proportional to the
number of data tuples.

In short, we don't need the object pools here and should make the computer stateless.


Line 93: 
TypeHelper.getNonOptionalType(...) can do the same thing, thus this method might not be necessary.


Line 105:         if (t.getTypeTag() == ATypeTag.ORDEREDLIST) {
TypeHelper.getNonOptionalType(...) can do the same thing, thus this method might not be necessary.


Line 123: 
TypeHelper.getNonOptionalType(...) can do the same thing, thus this method might not be necessary.


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?


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 and containing records?


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:
a type computer has to be stateless.


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

accessor is a legacy name because we used to call the interface IVisitableAccessor and the
interface was renamed to IVisitablePointable.


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