Till Westmann has posted comments on this change. Change subject: ASTERIXDB-1228: Add MISSING into the data model. ......................................................................
Patch Set 7: (50 comments) Looks generally good. Only tiny comments (but I've learned some things). https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-app/src/test/java/org/apache/asterix/runtime/NullMissingTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/runtime/NullMissingTest.java: Line 52: && !className.contains("SimilarityJaccardPrefix")) { Couldn't we just get the "right" list of functions from the FunctionCollection? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java: Line 467: if (aObjectType.getTypeTag() == ATypeTag.UNION) { One check too many ... https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/builders/AbstractListBuilder.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/builders/AbstractListBuilder.java: Line 88: data[start] = ATypeTag.SERIALIZED_NULL_TYPE_TAG; Would it be better to do this when writing to the output stream instead of modifying the content of the reference here? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AObjectAscBinaryComparatorFactory.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/comparators/AObjectAscBinaryComparatorFactory.java: Line 121: } Can we just put a conditional expression here? Line 136: } Can we just put a conditional expression here? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/adm/AObjectPrinter.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/adm/AObjectPrinter.java: Line 64: } Seems that we can combine the missing and null cases here ... https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/AObjectPrinter.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/csv/AObjectPrinter.java: Line 59: case MISSING: { It seems that we should also get null when printing CSVs ... https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/clean/AObjectPrinter.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/clean/AObjectPrinter.java: Line 61: case MISSING: { It seems that we should also get null when printing JSON ... https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/lossless/AObjectPrinter.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/dataflow/data/nontagged/printers/json/lossless/AObjectPrinter.java: Line 66: ANullPrinter.INSTANCE.print(b, s, l, ps); Seems that we can combine the missing and null cases here ... https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlADMPrinterFactoryProvider.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlADMPrinterFactoryProvider.java: Line 82: return ANullPrinterFactory.INSTANCE; Combine missing and null? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlBinaryBooleanInspectorImpl.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlBinaryBooleanInspectorImpl.java: Line 42: if (bytes[offset] == ATypeTag.SERIALIZED_MISSING_TYPE_TAG Might want to pull bytes[offset] out in case the JIT doesn't do this automatically .. https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlCSVPrinterFactoryProvider.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlCSVPrinterFactoryProvider.java: Line 78: return ANullPrinterFactory.INSTANCE; Combine missing and null? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlCleanJSONPrinterFactoryProvider.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlCleanJSONPrinterFactoryProvider.java: Line 82: return ANullPrinterFactory.INSTANCE; Combine missing and null? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlLosslessJSONPrinterFactoryProvider.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/formats/nontagged/AqlLosslessJSONPrinterFactoryProvider.java: Line 82: return ANullPrinterFactory.INSTANCE; Combine missing and null? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/adm/APrintVisitor.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/adm/APrintVisitor.java: Line 127: ANullPrinter.INSTANCE.print(b, s, l, ps); combine cases? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/csv/APrintVisitor.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/csv/APrintVisitor.java: Line 124: break; combine cases? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/json/clean/APrintVisitor.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/json/clean/APrintVisitor.java: Line 129: } combine cases? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/json/lossless/APrintVisitor.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/pointables/printer/json/lossless/APrintVisitor.java: Line 127: ANullPrinter.INSTANCE.print(b, s, l, ps); combine cases? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/base/AbstractResultTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/base/AbstractResultTypeComputer.java: Line 59: * @param knownInputTypes, Comment not updated? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/base/IResultTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/base/IResultTypeComputer.java: Line 28: Revert file? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/CollectionMemberResultType.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/CollectionMemberResultType.java: Line 38: if (type.getTypeTag() != ATypeTag.UNORDEREDLIST && type.getTypeTag() != ATypeTag.ORDEREDLIST) { Couldn't the argument be ANY as well? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NotMissingTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NotMissingTypeComputer.java: Line 36: * This class is the type computer for not-null function. fix comment https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NumericAddSubMulDivTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NumericAddSubMulDivTypeComputer.java: Line 209: return BuiltinType.ANY; Why do we return in some cases and assign a value to "type" in other cases? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NumericAggTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NumericAggTypeComputer.java: Line 66: case ANY: Is this correct? It seems that we might overflow easily. Are those the current semantics? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NumericRoundHalfToEven2TypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/NumericRoundHalfToEven2TypeComputer.java: Line 68: } Do we need to throw if argIndex > 1? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddFieldsTypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/RecordAddFieldsTypeComputer.java: Line 66: ARecordType inputRecordType = TypeComputeUtils.extractRecordType(type0); What is the difference between TypeComputeUtils and TypeComputerUtils? Seems a little confusing https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/ScalarVersionOfAggregateResultType.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/ScalarVersionOfAggregateResultType.java: Line 51: AbstractCollectionType act = (AbstractCollectionType) strippedInputTypes[0]; Pull this one to the top? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/TypeComputeUtils.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/TypeComputeUtils.java: Line 170: private static IAType getActualType(IAType inputType) { return inputType.getTypeTag() == ATypeTag.UNION ? ((AUnionType) inputType).getActualType() : inputType; ? Line 178: public static ARecordType extractRecordType(IAType t) { Should we just switch on the type tag in these methods? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/UnaryBinaryInt64TypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/UnaryBinaryInt64TypeComputer.java: Line 39: throw new AlgebricksException("The argument should be boolean Type."); s/boolean/binary/ https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/UnaryStringInt64TypeComputer.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/typecomputer/impl/UnaryStringInt64TypeComputer.java: Line 39: throw new AlgebricksException("Second argument should be integer Type."); Message seems wrong. https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/TypeHelper.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/TypeHelper.java: Line 29: AUnionType ut = (AUnionType) t; Can we inline this and remove all the curly braces in this method? Line 45: AUnionType ut = (AUnionType) t; Same as above ... Line 54: public static IAType getActualType(IAType t) { this is the same as TypeComputeUtils.getActualType Line 72: } Same as above https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/pom.xml File asterixdb/asterix-runtime/pom.xml: Line 86: </dependency> Do we have this in LICENSE/NOTICE? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/SqlSumAggregateFunction.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/SqlSumAggregateFunction.java: Line 65: } revert file? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/SumAggregateFunction.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/aggregates/std/SumAggregateFunction.java: Line 70: } revert file? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/EditDistanceCheckEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/common/EditDistanceCheckEvaluator.java: Line 85: editDistance = computeResult(argPtr1, argPtr2, firstTypeTag); Maybe one big try-catch for "HyracksDataException|IOException"? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractComparisonEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/comparisons/AbstractComparisonEvaluator.java: Line 96: } Pull serialization after the if? Line 130: byte tagByte2 = argRight.getTag(); inline these 2? Line 140: } "return ATypeHierarchy.isCompatible(typeTag1, typeTag2)"? Line 147: return result; inline? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/EditDistanceStringIsFilterableDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/EditDistanceStringIsFilterableDescriptor.java: Line 36: * are fed into a (non indexed) nested-loop join. line length? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/binary/AbstractSubBinaryEvaluator.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/functions/binary/AbstractSubBinaryEvaluator.java: Line 93: throw new AlgebricksException(e); Remove this first catch? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/staticcodegen/CodeGenUtil.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/staticcodegen/CodeGenUtil.java: Line 41: private final static String DESCRIPTER_SUPER_CLASS_NAME = "org/apache/asterix/runtime/evaluators/base/AbstractScalarFunctionDynamicDescriptor"; s/DESCRIPTER/DESCRIPTOR/ line too long https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/staticcodegen/TypeChecker.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/evaluators/staticcodegen/TypeChecker.java: Line 45: if (data[start] == ATypeTag.SERIALIZED_MISSING_TYPE_TAG) { Pull data[start] out to make life easier for the compiler? https://asterix-gerrit.ics.uci.edu/#/c/846/7/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/formats/NonTaggedDataFormat.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/formats/NonTaggedDataFormat.java: Line 406: "reset(org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType)", line too long Line 441: "reset(org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType)", line too long Line 463: "reset(org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType)", line too long -- To view, visit https://asterix-gerrit.ics.uci.edu/846 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia49ed8474bfc5d6604231819065117468c5b0897 Gerrit-PatchSet: 7 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 <michael.b...@couchbase.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
