asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Unify runtime type exceptions by using error code and messag...
Date Mon, 31 Oct 2016 17:48:29 GMT
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 <buyingyi@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Xikui Wang <xkkwww@gmail.com>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message