>From Wael Alkowaileet <[email protected]>: Wael Alkowaileet 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 20: (11 comments) 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"? 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 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) 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? 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.getStartOffset()] 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. But below, it does not. 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. Is the value tagged or untagged? 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? 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? 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? 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 -- 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: 20 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: Sun, 03 Oct 2021 16:31:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
