Return-Path: X-Original-To: apmail-asterixdb-notifications-archive@minotaur.apache.org Delivered-To: apmail-asterixdb-notifications-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 8814B18472 for ; Thu, 19 Nov 2015 20:05:59 +0000 (UTC) Received: (qmail 20764 invoked by uid 500); 19 Nov 2015 20:05:59 -0000 Delivered-To: apmail-asterixdb-notifications-archive@asterixdb.apache.org Received: (qmail 20731 invoked by uid 500); 19 Nov 2015 20:05:59 -0000 Mailing-List: contact notifications-help@asterixdb.incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@asterixdb.incubator.apache.org Delivered-To: mailing list notifications@asterixdb.incubator.apache.org Received: (qmail 20722 invoked by uid 99); 19 Nov 2015 20:05:59 -0000 Received: from Unknown (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 19 Nov 2015 20:05:59 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id F07B31A20D3 for ; Thu, 19 Nov 2015 20:05:58 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 0.92 X-Spam-Level: X-Spam-Status: No, score=0.92 tagged_above=-999 required=6.31 tests=[SPF_FAIL=0.919, URIBL_BLOCKED=0.001] autolearn=disabled Received: from mx1-us-east.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id QFfsifSA3mDO for ; Thu, 19 Nov 2015 20:05:50 +0000 (UTC) Received: from unhygienix.ics.uci.edu (unhygienix.ics.uci.edu [128.195.14.130]) by mx1-us-east.apache.org (ASF Mail Server at mx1-us-east.apache.org) with ESMTP id 5CD12439F8 for ; Thu, 19 Nov 2015 20:05:50 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by unhygienix.ics.uci.edu (Postfix) with ESMTP id 17163240D69; Thu, 19 Nov 2015 12:02:42 -0800 (PST) Date: Thu, 19 Nov 2015 12:02:42 -0800 From: "Yingyi Bu (Code Review)" To: Heri Ramampiaro CC: Steven Jacobs , Preston Carman , Ian Maxon , Jenkins , Till Westmann Reply-To: buyingyi@gmail.com X-Gerrit-MessageType: comment Subject: Change in asterixdb[master]: Internal functions for record manipulation, deep equality co... X-Gerrit-Change-Id: I3621ebdd71c7cd91b50d77a972ad863cea7fcbc2 X-Gerrit-ChangeURL: X-Gerrit-Commit: 4e4233dc78dc44f2c3fb6508391cdb8e363d917f In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/2.8.4 Message-Id: <20151119200242.17163240D69@unhygienix.ics.uci.edu> 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> { 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> 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 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 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 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