asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Preston Carman (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Refactored the AbstractComparisonEvaluator.
Date Sun, 17 Apr 2016 23:57:15 GMT
Preston Carman has posted comments on this change.

Change subject: Refactored the AbstractComparisonEvaluator.
......................................................................


Patch Set 1:

(15 comments)

https://asterix-gerrit.ics.uci.edu/#/c/801/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/nonvisitor/AIntervalPointable.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/nonvisitor/AIntervalPointable.java:

Line 113:                 throw new IllegalStateException("Unsopported interval type.");
> s/Unsopported/Unsupported/
Done


Line 125:                 throw new IllegalStateException("Unsopported interval type.");
> s/Unsopported/Unsupported/
Done


Line 145:                 throw new IllegalStateException("Unsopported interval type.");
> s/Unsopported/Unsupported/
Done


Line 157:                 throw new IllegalStateException("Unsopported interval type.");
> s/Unsopported/Unsupported/
Done


https://asterix-gerrit.ics.uci.edu/#/c/801/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/ComparisonUtil.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/ComparisonUtil.java:

Line 52:     protected final IBinaryComparator strBinaryComp = AqlBinaryComparatorFactoryProvider.UTF8STRING_POINTABLE_INSTANCE
> Could those be private?
Done


Line 81:             }
> Can we remove the curly braces here?
Done


Line 436:     private final int compareByte(int v1, int v2) {
> I think this is more clear : -1, 0, 1. :-)
Either work, but this option does not change the existing code.


https://asterix-gerrit.ics.uci.edu/#/c/801/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/AbstractIntervalLogicFuncDescriptor.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/AbstractIntervalLogicFuncDescriptor.java:

Line 107:                             if (interval0.getType() != interval0.getType()) {
> Maybe interval0.getType() != interval1.getType() ???
Done


https://asterix-gerrit.ics.uci.edu/#/c/801/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/GetOverlappingIntervalDescriptor.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/GetOverlappingIntervalDescriptor.java:

Line 105:                                 if (interval0.getType() != interval0.getType())
{
> An independent comment: maybe interval0.getType() != interval1.getType() ??
Done


Line 105:                                 if (interval0.getType() != interval0.getType())
{
> There might still be a point of putting those into local variables as the t
Done


https://asterix-gerrit.ics.uci.edu/#/c/801/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/IntervalLogic.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/IntervalLogic.java:

Line 57:         ip2.getEnd(e2);
> Seems that only e1 and s2 are needed.
Done


Line 78:         ip2.getEnd(e2);
> See above
Done


https://asterix-gerrit.ics.uci.edu/#/c/801/1/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/IntervalMetByDescriptor.java
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/IntervalMetByDescriptor.java:

Line 45:     public FunctionIdentifier getIdentifier() {
> It seems that FID could either be a final member of the superclass (in whic
Done


https://asterix-gerrit.ics.uci.edu/#/c/801/1/hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/api/IComparable.java
File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/api/IComparable.java:

Line 22:     public int compareTo(byte[] bytes, int start, int length);
> Are you sure it's ok to remove this Hyracks interface? Seems risky for othe
Done


https://asterix-gerrit.ics.uci.edu/#/c/801/1/hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/primitive/TaggedValuePointable.java
File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/primitive/TaggedValuePointable.java:

Line 26: public class TaggedValuePointable extends AbstractPointable {
> Hyracks should not know the existence of type tag, right? Maybe this class 
I was debating where this should be located. We have this class in VXQuery and its needed
in AsterixDB. Its the same for both systems using Hyracks. Should it be duplicated or kept
out of Hyracks? I am not sure what the correct answer.


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/801
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I42e0e8cf71207bb862334cd0629e8c024ff0556c
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Preston Carman <prestonc@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Preston Carman <prestonc@apache.org>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message