Preston Carman has posted comments on this change. Change subject: Updated DeletableFrameTupleAppender to support reusing index slots. ......................................................................
Patch Set 1: (9 comments) https://asterix-gerrit.ics.uci.edu/#/c/1214/1/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/util/DeletableFrameTupleAppender.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/util/DeletableFrameTupleAppender.java: Line 34: * The offsets also store both the start and end values because tuples may be out of > out of "range"? Done Line 41: * <li><code>tuple_append</code> holds an int indicating the offset used to append the next tuple.</li> > maybe it helps to explain the difference between `tuple_append` and the `ne Added more detail on all the frame fields. Line 79: private IntegerPairPool ipp = new IntegerPairPool(); > it's better to unify the initialization place, either all inside constructo Done Line 80: > ipp should be final variable as well? Done Line 88: private int getIndexCount() { > it's often more clear to move the private functions to the end. I am hesitant to change the order since that will increase the challenge in reviewing this code. Let me post your changes and then we can do a second patch with private functions at the end. Line 128: if (getTupleEndOffset(nextIndex) <= 0) { > is it possible to merge this condition with the while loop? Done Line 160: setTupleAppend(0); > If I understand correctly, this offset should add some paddings? The tuple append is relative to the start of the frame. Since the metadata is a the end of the frame, you only need to make sure the tuple append value never overlaps the growing metadata. Line 279: } > do we need to set the member variable `nextIndex` as well? Yes, that would be needed. As I thought about it, this use case never happens. I removed the condition. https://asterix-gerrit.ics.uci.edu/#/c/1214/1/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/util/IntegerPairPool.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/util/IntegerPairPool.java: Line 26: list = new ArrayList<>(); > since the behavior is just adding and removing to and from tail, it would b Done -- To view, visit https://asterix-gerrit.ics.uci.edu/1214 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I05cda3360cc503bf847aeca5998be06f1f7d3c15 Gerrit-PatchSet: 1 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Preston Carman <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Jianfeng Jia <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
