asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Preston Carman (Code Review)" <>
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:

File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/nonvisitor/

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

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

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

Line 157:                 throw new IllegalStateException("Unsopported interval type.");
> s/Unsopported/Unsupported/
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/

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

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

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.
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/

Line 107:                             if (interval0.getType() != interval0.getType()) {
> Maybe interval0.getType() != interval1.getType() ???
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/

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

Line 105:                                 if (interval0.getType() != interval0.getType())
> There might still be a point of putting those into local variables as the t
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/

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

Line 78:         ip2.getEnd(e2);
> See above
File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/

Line 45:     public FunctionIdentifier getIdentifier() {
> It seems that FID could either be a final member of the superclass (in whic
File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/api/

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
File hyracks-fullstack/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/primitive/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I42e0e8cf71207bb862334cd0629e8c024ff0556c
Gerrit-PatchSet: 1
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Preston Carman <>
Gerrit-Reviewer: Jenkins <>
Gerrit-Reviewer: Preston Carman <>
Gerrit-Reviewer: Taewoo Kim <>
Gerrit-Reviewer: Till Westmann <>
Gerrit-HasComments: Yes

View raw message