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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
