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]: ASTERIXDB-1187 and ASTERIXDB-1162 fixes, as well as updates ...
Date Mon, 14 Dec 2015 23:23:42 GMT
Yingyi Bu has posted comments on this change.

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


Patch Set 16:

(8 comments)

Detailed comments are inlined.
BTW, it seems the CodeStyle is still different from what I have. If I format your code in
my Eclipse, I still can see the files are changed...

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

Line 56:     private final ByteArrayAccessibleOutputStream openPartOutputStream;
These fields can be moved to specific functions which need to dynamically check duplicate
fields.


Line 187:         if (arg.first == true)
The dynamic check in line 182 to line 188 seems only being used by record-merge function and
record-add-field function because the compiler couldn't statically check that.

RecordBuilders are used in almost every query and might be called for *every* tuple, e.g.,
in the return clause.  For those usual cases, the check already happens in the compiler, hence
this runtime check is not needed and will add some overheads.


https://asterix-gerrit.ics.uci.edu/#/c/298/16/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 45:     public static IAType mergedNestedType(IAType fieldType1, IAType fieldType0) throws
AlgebricksException,
name those parameters properly, e.g.:
mergeNestedType --> mergeNestedRecordType
fieldType1-->nestedRecordTypeLeft
fieldType2-->nestedRecordTypeRight


Line 48:             throw new AlgebricksException("Duplicate field " + fieldType1.getTypeName()
+ " encountered");
The exception message seems not accurate enough.


https://asterix-gerrit.ics.uci.edu/#/c/298/16/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/DeepEqualAssessor.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/DeepEqualAssessor.java:

Line 66:                         return true;
wrap a single line if-else-block with {}.


Line 74:                 return false;
wrap a single line if-else-block with {}.


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

Line 49:     private final AMutableString aString = new AMutableString("");
The same comments can be found in the other code review:
https://asterix-gerrit.ics.uci.edu/#/c/523/3/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableUtils.java


https://asterix-gerrit.ics.uci.edu/#/c/298/16/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 60:     private final Deque<IVisitablePointable> recordPath = new ArrayDeque<>();
Potential race conditions: recordPath is shared by multiple partitions.


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