Yingyi Bu has posted comments on this change. Change subject: ASTERIXDB-1228: Add MISSING into the data model. ......................................................................
Patch Set 7: (49 comments) 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 FunctionCollect Done 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 ... Done 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 Done 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? Done Line 136: } > Can we just put a conditional expression here? Done 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 ... Done 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 ... Done 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 ... Done 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 ... Done 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? Done 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 automa Done 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? Done 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? Done 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? Done 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? Done 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? Done 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? Done 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? Done 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? Done 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? Done 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? ANY, MISSING, NULL are handled in TypeComputeUtils (line 86). Therefore, a type computer impl doesn't need to think of them. 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 Done 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? Done 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 curr Yes, they are 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? Here we don't need to consider that. The function arity has already been checked at function lookup time: E.g., round-half-to-even(1.02, 2, 3); org.apache.asterix.common.exceptions.AsterixException: function null.round-half-to-even@3 is undefined at org.apache.asterix.lang.aql.rewrites.AqlQueryRewriter.buildOtherUdfs(AqlQueryRewriter.java:150) at org.apache.asterix.lang.aql.rewrites.AqlQueryRewriter.inlineDeclaredUdfs(AqlQueryRewriter.java:112) at org.apache.asterix.lang.aql.rewrites.AqlQueryRewriter.rewrite(AqlQueryRewriter.java:80) at org.apache.asterix.api.common.APIFramework.reWriteQuery(APIFramework.java:194) at org.apache.asterix.aql.translator.QueryTranslator.rewriteCompileQuery(QueryTranslator.java:1955) at org.apache.asterix.aql.translator.QueryTranslator.handleQuery(QueryTranslator.java:2517) at org.apache.asterix.aql.translator.QueryTranslator.compileAndExecute(QueryTranslator.java:389) at org.apache.asterix.aql.translator.QueryTranslator.compileAndExecute(QueryTranslator.java:253) at org.apache.asterix.api.http.servlet.APIServlet.doPost(APIServlet.java:148) at javax.servlet.http.HttpServlet.service(HttpServlet.java:755) at javax.servlet.http.HttpServlet.service(HttpServlet.java:848) at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:546) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:483) at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:231) at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:970) at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:411) at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:192) at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:904) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117) at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:110) at org.eclipse.jetty.server.Server.handle(Server.java:347) at org.eclipse.jetty.server.HttpConnection.handleRequest(HttpConnection.java:439) at org.eclipse.jetty.server.HttpConnection$RequestHandler.content(HttpConnection.java:924) at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:781) at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:220) at org.eclipse.jetty.server.AsyncHttpConnection.handle(AsyncHttpConnection.java:43) at org.eclipse.jetty.io.nio.SelectChannelEndPoint.handle(SelectChannelEndPoint.java:545) at org.eclipse.jetty.io.nio.SelectChannelEndPoint$1.run(SelectChannelEndPoint.java:43) at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:529) 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? TypeComputerUtilities is only used for type casting. I have renamed TypeComputerUtilities to TypeCastUtils. 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? Done 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). Done Line 178: public static ARecordType extractRecordType(IAType t) { > Should we just switch on the type tag in these methods? Done 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/ Done 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. Done 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? Done Line 45: AUnionType ut = (AUnionType) t; > Same as above ... Done Line 54: public static IAType getActualType(IAType t) { > this is the same as TypeComputeUtils.getActualType Done Line 72: } > Same as above Done 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? Done 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? Done 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? Done 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"? Done 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 130: byte tagByte2 = argRight.getTag(); > inline these 2? Done Line 140: } > "return ATypeHierarchy.isCompatible(typeTag1, typeTag2)"? Done Line 147: return result; > inline? Done 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? Done 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? Done 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/ Done 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? Done 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 Done Line 441: "reset(org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType)", > line too long Done Line 463: "reset(org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType, org.apache.asterix.om.types.IAType)", > line too long Done -- 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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
