>From Ian Maxon <[email protected]>: Ian Maxon has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643 )
Change subject: [ASTERIXDB-2895][RT] Vsize buffers in PyUDF IPC ...................................................................... Patch Set 22: (11 comments) packing refactored to use IVisitablePointable instead of bespoke traversal https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/builders/StdToModUTF8DataOutputFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/builders/StdToModUTF8DataOutputFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/builders/StdToModUTF8DataOutputFactory.java@28 PS20, Line 28: StdToModUTF8DataOutputFactory > Minor: Maybe use the full "StandardToModifiedUTF8DataOutputFactory"? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/ipc/PythonMessageBuilder.java@47 PS20, Line 47: position > clear() will make position=0 and limit=capacity Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalScalarPythonFunctionEvaluator.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalScalarPythonFunctionEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/ExternalScalarPythonFunctionEvaluator.java@138 PS20, Line 138: Returned result missing outer wrapper > Maybe move the message to ErrorCode instead of hard coded (for localization) Done. Not really the right exception there anyway, code 201 is for when the UDF itself errors out and the string is supposed to be the content of what it produced. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@77 PS20, Line 77: private final CharsetEncoder encoder; : private final CharBuffer cbuf; > Are these still used? nope, removed. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@91 PS20, Line 91: TaggedValuePointable pointy = TaggedValuePointable.FACTORY.createPointable(); > Is this needed? You can get the type tag from ptr.getByteArray()[ptr. […] nope not for any reason other than looking at the tag https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@95 PS20, Line 95: pack > Does it expect the value to be tagged or untagged? The value here (from ptr) > contains the type tag. […] this should be the tagged branch https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@97 PS20, Line 97: true > The value is not tagged? yes. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@97 PS20, Line 97: pack > Same. […] here this should be untagged. the type is given. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@152 PS20, Line 152: PARSER_ADM_DATA_PARSER_CAST_ERROR > Maybe ErrorCode#TYPE_UNSUPPORTED instead? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@264 PS20, Line 264: cbuf.clear(); : cbuf.position(0); > Still needed? nope, removed. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643/20/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/library/msgpack/MessagePackerFromADM.java@306 PS20, Line 306: getClosedFieldOffset > Need to inspect the null bitmap to check whether the value is null/missing good catch, done -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/12643 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: Ic95e592b42139b4750af8bb20291f926b3c973e2 Gerrit-Change-Number: 12643 Gerrit-PatchSet: 22 Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Wael Alkowaileet <[email protected]> Gerrit-Comment-Date: Tue, 26 Oct 2021 00:55:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Wael Alkowaileet <[email protected]> Gerrit-MessageType: comment
