Yingyi Bu has posted comments on this change. Change subject: Unify runtime type exceptions by using error code and message template. ......................................................................
Patch Set 10: (20 comments) https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/RuntimeDataException.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/RuntimeDataException.java: Line 26: public class RuntimeDataException extends HyracksDataException { > I think to fix that, we should get rid of HyracksDataException and just kee HyracksDataException will become CoreDataException. We can discuss the proper name. HyracksException is still needed for things not thrown from data -- maybe we should rename it to CoreException. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalFunctionProvider.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalFunctionProvider.java: Line 55: initialize(functionHelper); > let's change the initialize interface to throw HyracksDataException only. I tried to do this. But it will change so many classes in asterix-external, beyond what I can handle in one change. Xikui offered to clean up this module. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlBinaryIntegerInspector.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlBinaryIntegerInspector.java: Line 42: public int getIntegerValue(byte[] bytes, int offset, int length) throws HyracksDataException { > all other callers reference a function name when calling this method. what This is used in LIMIT and SPLIT operator, the runtime hyracks operator needs a way to interpret the evaluation result of an expression. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/serializable/std/AbstractSerializableAvgAggregateFunction.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/serializable/std/AbstractSerializableAvgAggregateFunction.java: Line 259: protected void finishFinalResults(byte[] state, int start, int len, DataOutput result) throws HyracksDataException { > address this. Len is potentially useful in implementation subclasses. This is an abstract class. Every other counterpart method have len. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/AsterixListAccessor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/AsterixListAccessor.java: Line 60: if (listBytes[start] != ATypeTag.SERIALIZED_UNORDEREDLIST_TYPE_TAG > Pull this out? Done Line 60: if (listBytes[start] != ATypeTag.SERIALIZED_UNORDEREDLIST_TYPE_TAG > Pull this out? Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/DeepEqualAssessor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/DeepEqualAssessor.java: Line 60: double leftVal = ATypeHierarchy.getDoubleValue("deep-equal", 0, leftPointable.getByteArray(), > constant for this? Done Line 64: return Math.abs(leftVal - rightVal) < 1E-10; > constant for this? Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/constructors/AInt64ConstructorDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/constructors/AInt64ConstructorDescriptor.java: Line 108: } > replace this with Long.MIN_VALUE ?? Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractNumericArithmeticEval.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/AbstractNumericArithmeticEval.java: Line 218: switch (resultType) { > address this Actually, there is no "default" case, as the resultType is assigned at line 211. For a type is not belong to those enums, it either returns at line 197 or line 199. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/NumericSubtractDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/NumericSubtractDescriptor.java: > remove braces for switch cases or move them to methods. Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/PointableHelper.java: Line 115: */ > this doesn't match the method signature. Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/SpatialIntersectDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/SpatialIntersectDescriptor.java: > these are really too long case statements. refactor? This change only intends to clean up exceptions. Refactoring this class is beyond the scope of this change. Opened ASTERIXDB-1722 for this. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/FieldAccessNestedEvalFactory.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/records/FieldAccessNestedEvalFactory.java: Line 234: && serRecord[start] != ATypeTag.SERIALIZED_RECORD_TYPE_TAG) { > CRITICAL SonarQube violation: Done Line 247: } catch (IOException e) { > change to multi exception catch. Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/ParseDateDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/ParseDateDescriptor.java: Line 134: } catch (AsterixTemporalTypeParseException ex) { > this is some really scary stuff......... I'm not sure why temporal parsers work like this. This is beyond the scope of this change. No regression from this change. Opened ASTERIXDB-1721 for this. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/ParseDateTimeDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/ParseDateTimeDescriptor.java: Line 128: } catch (AsterixTemporalTypeParseException ex) { > again, scary stuff........ Beyond the scope of this change. No regression from this change. Opened ASTERIXDB-1721 for this. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/ParseTimeDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/temporal/ParseTimeDescriptor.java: Line 129: } catch (AsterixTemporalTypeParseException ex) { > !!!!! Beyond the scope of this change. No regression from this change. Opened ASTERIXDB-1721 for this. https://asterix-gerrit.ics.uci.edu/#/c/1313/10/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/exceptions/ExceptionUtil.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/exceptions/ExceptionUtil.java: Line 47: return "1st"; > what about 3rd, 21st, 22nd, 23rd, 31st, 32nd, 33rd, etc. Done https://asterix-gerrit.ics.uci.edu/#/c/1313/10/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/NestedLoopJoinPOperator.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/physical/NestedLoopJoinPOperator.java: Line 213: condEvaluator.evaluate(compositeTupleRef, p); > SonarQube got confused because of the mixed referencing (with this. and wit Done -- To view, visit https://asterix-gerrit.ics.uci.edu/1313 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie4fff8f5e64ffb027910a4899c0246b37ed5bce7 Gerrit-PatchSet: 10 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Yingyi Bu <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
