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 <[email protected]> Gerrit-Reviewer: Heri Ramampiaro <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
