abdullah alamoudi has posted comments on this change. Change subject: Unify runtime type exceptions by using error code and message template. ......................................................................
Patch Set 10: (18 comments) Looks good to me. a few comments. keeping in mind that this is just a step in the whole exception handling story. 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 { > MAJOR SonarQube violation: I think to fix that, we should get rid of HyracksDataException and just keep HyracksException. also maybe not extend IOException since we encapsulate it now. Not necessarily in this change 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. We can defer this to another change. your call. 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 is special about this one? when is it used? 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 { > MAJOR SonarQube violation: address this. 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 ?? 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: remove braces from switch statements? or move them to methods Line 218: switch (resultType) { > MAJOR SonarQube violation: address this 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. 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. 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? 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: take care of this one. Line 247: } catch (IOException e) { change to multi exception catch. 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) { > CRITICAL SonarQube violation: this is some really scary stuff......... 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) { > CRITICAL SonarQube violation: again, scary stuff........ 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) { > CRITICAL SonarQube violation: !!!!! 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. how about: static String indexToPosition(int index) { int i = index + 1; switch (i % 100) { case 11: case 12: case 13: return i + "th"; default: switch (i % 10) { case 1: return i + "st"; case 2: return i + "nd"; case 3: return i + "rd"; default: return i + "th"; } } } works for all cases. 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); > BLOCKER SonarQube violation: SonarQube got confused because of the mixed referencing (with this. and without this.) let's remove this when it is not needed and maybe also move this assignment to the constructor? then we don't need to keep references to the factory or the context https://asterix-gerrit.ics.uci.edu/#/c/1313/10/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/SubplanRuntimeFactory.java File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/SubplanRuntimeFactory.java: Line 162: try { (Y) -- 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 <buyin...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
