sashapolo commented on code in PR #2728:
URL: https://github.com/apache/ignite-3/pull/2728#discussion_r1369829525
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/BinaryTuplePrefix.java:
##########
@@ -51,13 +52,36 @@ public BinaryTuplePrefix(int elementCount, ByteBuffer
buffer) {
super(elementCount, buffer);
}
+ /**
+ * Creates a prefix from provided {@link BinaryTuple}. If given tuple has
lesser or equal number of
+ * columns, then all elements will be used in resulting prefix. If given
tuple has more columns, then
+ * excess columns will be truncated.
+ *
+ * @param size The size of the complete tuple.
Review Comment:
I don't think `size` is a good name, maybe `numElements` is better. Also, I
think `truncateTuple` is a better name for this method (but that's optional)
##########
modules/binary-tuple/src/main/java/org/apache/ignite/internal/binarytuple/BinaryTupleBuilder.java:
##########
@@ -82,11 +82,24 @@ public BinaryTupleBuilder(int numElements) {
* @param totalValueSize Total estimated length of non-NULL values, -1 if
not known.
*/
public BinaryTupleBuilder(int numElements, int totalValueSize) {
+ this(numElements, totalValueSize, true);
+ }
+
+ /**
+ * Creates a builder.
+ *
+ * @param numElements Number of tuple elements.
+ * @param totalValueSize Total estimated length of non-NULL values, -1 if
not known.
+ * @param exactEstimate Whether the total size is exact estimate or
approximate. The
+ * difference here is with exact estimate allocation will be optimal,
while with
+ * approximate estimate some excess allocation is possible.
+ */
+ public BinaryTupleBuilder(int numElements, int totalValueSize, boolean
exactEstimate) {
Review Comment:
Why do we need this flag? Can we pass `-1` as `totalValueSize` instead, if
needed?
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/BinaryTuplePrefix.java:
##########
@@ -73,6 +97,102 @@ public static BinaryTuplePrefix fromBinaryTuple(BinaryTuple
tuple) {
return new BinaryTuplePrefix(tuple.elementCount(), prefixBuffer);
}
+ private static BinaryTuplePrefix expandTuple(int size, BinaryTuple tuple) {
+ assert size > tuple.elementCount();
+
+ var stats = new Sink() {
+ int dataBeginOffset = 0;
+ int dataEndOffset = 0;
+
+ @Override
+ public void nextElement(int index, int begin, int end) {
+ if (index == 0) {
+ dataBeginOffset = begin;
+ }
+
+ dataEndOffset = end;
+ }
+ };
+
+ tuple.parse(stats);
+
+ ByteBuffer tupleBuffer = tuple.byteBuffer();
+
+ byte flags = tupleBuffer.get(0);
+ int entrySize = BinaryTupleCommon.flagsToEntrySize(flags);
+
+ ByteBuffer prefixBuffer = ByteBuffer.allocate(
+ tupleBuffer.remaining()
+ + (entrySize * (size - tuple.elementCount()))
+ + Integer.BYTES)
+ .order(ORDER)
+ .put(tupleBuffer.slice().limit(stats.dataBeginOffset)); //
header
+
+ int payloadEndPosition = stats.dataEndOffset - stats.dataBeginOffset;
+ for (int idx = tuple.elementCount(); idx < size; idx++) {
+ switch (entrySize) {
+ case Byte.BYTES:
+ prefixBuffer.put((byte) payloadEndPosition);
+ break;
+ case Short.BYTES:
+ prefixBuffer.putShort((short) payloadEndPosition);
+ break;
+ case Integer.BYTES:
+ prefixBuffer.putInt(payloadEndPosition);
+ break;
+ default:
+ assert false;
+ }
+ }
+
+ prefixBuffer
+
.put(tupleBuffer.slice().position(stats.dataBeginOffset).limit(stats.dataEndOffset))
// payload
+ .putInt(tuple.elementCount())
+ .flip();
+
+ prefixBuffer.put(0, (byte) (flags | PREFIX_FLAG));
+
+ return new BinaryTuplePrefix(size, prefixBuffer);
+ }
+
+ private static BinaryTuplePrefix truncateTuple(int size, BinaryTuple
tuple) {
+ assert size < tuple.elementCount();
+
+ var stats = new Sink() {
+ int dataBeginOffset = 0;
+ int dataEndOffset = 0;
+
+ @Override
+ public void nextElement(int index, int begin, int end) {
+ if (index == 0) {
+ dataBeginOffset = begin;
+ }
+
+ if (index < size) {
+ dataEndOffset = end;
+ }
+ }
+ };
+
+ tuple.parse(stats);
Review Comment:
This doesn't look optimal to me. Why do we have to iterate over the whole
tuple just to get the start and end of the data range? I think we can use a
different API (this is pseudocode):
```
tuple.fetch(0, (index, begin, end) -> dataBeginOffset = begin);
tuple.fetch(size - 1, (index, begin, end) -> dataBeginOffset = end);
```
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/BinaryTuplePrefix.java:
##########
@@ -73,6 +97,102 @@ public static BinaryTuplePrefix fromBinaryTuple(BinaryTuple
tuple) {
return new BinaryTuplePrefix(tuple.elementCount(), prefixBuffer);
}
+ private static BinaryTuplePrefix expandTuple(int size, BinaryTuple tuple) {
+ assert size > tuple.elementCount();
+
+ var stats = new Sink() {
+ int dataBeginOffset = 0;
+ int dataEndOffset = 0;
+
+ @Override
+ public void nextElement(int index, int begin, int end) {
+ if (index == 0) {
+ dataBeginOffset = begin;
+ }
+
+ dataEndOffset = end;
+ }
+ };
+
+ tuple.parse(stats);
+
+ ByteBuffer tupleBuffer = tuple.byteBuffer();
+
+ byte flags = tupleBuffer.get(0);
+ int entrySize = BinaryTupleCommon.flagsToEntrySize(flags);
+
+ ByteBuffer prefixBuffer = ByteBuffer.allocate(
+ tupleBuffer.remaining()
+ + (entrySize * (size - tuple.elementCount()))
+ + Integer.BYTES)
+ .order(ORDER)
+ .put(tupleBuffer.slice().limit(stats.dataBeginOffset)); //
header
+
+ int payloadEndPosition = stats.dataEndOffset - stats.dataBeginOffset;
+ for (int idx = tuple.elementCount(); idx < size; idx++) {
+ switch (entrySize) {
+ case Byte.BYTES:
+ prefixBuffer.put((byte) payloadEndPosition);
+ break;
+ case Short.BYTES:
+ prefixBuffer.putShort((short) payloadEndPosition);
+ break;
+ case Integer.BYTES:
+ prefixBuffer.putInt(payloadEndPosition);
+ break;
+ default:
+ assert false;
+ }
+ }
+
+ prefixBuffer
+
.put(tupleBuffer.slice().position(stats.dataBeginOffset).limit(stats.dataEndOffset))
// payload
+ .putInt(tuple.elementCount())
+ .flip();
+
+ prefixBuffer.put(0, (byte) (flags | PREFIX_FLAG));
+
+ return new BinaryTuplePrefix(size, prefixBuffer);
+ }
+
+ private static BinaryTuplePrefix truncateTuple(int size, BinaryTuple
tuple) {
+ assert size < tuple.elementCount();
+
+ var stats = new Sink() {
+ int dataBeginOffset = 0;
+ int dataEndOffset = 0;
+
+ @Override
+ public void nextElement(int index, int begin, int end) {
+ if (index == 0) {
+ dataBeginOffset = begin;
+ }
+
+ if (index < size) {
+ dataEndOffset = end;
+ }
+ }
+ };
+
+ tuple.parse(stats);
+
+ BinaryTuplePrefixBuilder builder = new BinaryTuplePrefixBuilder(size,
size, stats.dataEndOffset - stats.dataBeginOffset);
+
+ tuple.parse((index, begin, end) -> {
Review Comment:
BTW, maybe we could introduce a more effective API to the BinaryTuple,
because in this approach we do a lot of array copying
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/BinaryTuplePrefix.java:
##########
@@ -51,13 +52,36 @@ public BinaryTuplePrefix(int elementCount, ByteBuffer
buffer) {
super(elementCount, buffer);
}
+ /**
+ * Creates a prefix from provided {@link BinaryTuple}. If given tuple has
lesser or equal number of
+ * columns, then all elements will be used in resulting prefix. If given
tuple has more columns, then
+ * excess columns will be truncated.
+ *
+ * @param size The size of the complete tuple.
+ * @param tuple Tuple to create a prefix from.
+ * @return Prefix, created from provided tuple with regards to desired
size.
+ */
+ public static BinaryTuplePrefix fromBinaryTuple(int size, BinaryTuple
tuple) {
+ if (size == tuple.elementCount()) {
+ return entireTuple(tuple);
+ } else if (size > tuple.elementCount()) {
Review Comment:
Why do we need this branch?
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/BinaryTuplePrefix.java:
##########
@@ -73,6 +97,102 @@ public static BinaryTuplePrefix fromBinaryTuple(BinaryTuple
tuple) {
return new BinaryTuplePrefix(tuple.elementCount(), prefixBuffer);
}
+ private static BinaryTuplePrefix expandTuple(int size, BinaryTuple tuple) {
+ assert size > tuple.elementCount();
+
+ var stats = new Sink() {
+ int dataBeginOffset = 0;
+ int dataEndOffset = 0;
+
+ @Override
+ public void nextElement(int index, int begin, int end) {
+ if (index == 0) {
+ dataBeginOffset = begin;
+ }
+
+ dataEndOffset = end;
+ }
+ };
+
+ tuple.parse(stats);
+
+ ByteBuffer tupleBuffer = tuple.byteBuffer();
+
+ byte flags = tupleBuffer.get(0);
+ int entrySize = BinaryTupleCommon.flagsToEntrySize(flags);
+
+ ByteBuffer prefixBuffer = ByteBuffer.allocate(
+ tupleBuffer.remaining()
+ + (entrySize * (size - tuple.elementCount()))
+ + Integer.BYTES)
+ .order(ORDER)
+ .put(tupleBuffer.slice().limit(stats.dataBeginOffset)); //
header
+
+ int payloadEndPosition = stats.dataEndOffset - stats.dataBeginOffset;
+ for (int idx = tuple.elementCount(); idx < size; idx++) {
+ switch (entrySize) {
+ case Byte.BYTES:
+ prefixBuffer.put((byte) payloadEndPosition);
+ break;
+ case Short.BYTES:
+ prefixBuffer.putShort((short) payloadEndPosition);
+ break;
+ case Integer.BYTES:
+ prefixBuffer.putInt(payloadEndPosition);
+ break;
+ default:
+ assert false;
+ }
+ }
+
+ prefixBuffer
+
.put(tupleBuffer.slice().position(stats.dataBeginOffset).limit(stats.dataEndOffset))
// payload
+ .putInt(tuple.elementCount())
+ .flip();
+
+ prefixBuffer.put(0, (byte) (flags | PREFIX_FLAG));
+
+ return new BinaryTuplePrefix(size, prefixBuffer);
+ }
+
+ private static BinaryTuplePrefix truncateTuple(int size, BinaryTuple
tuple) {
+ assert size < tuple.elementCount();
+
+ var stats = new Sink() {
+ int dataBeginOffset = 0;
+ int dataEndOffset = 0;
+
+ @Override
+ public void nextElement(int index, int begin, int end) {
+ if (index == 0) {
+ dataBeginOffset = begin;
+ }
+
+ if (index < size) {
+ dataEndOffset = end;
+ }
+ }
+ };
+
+ tuple.parse(stats);
+
+ BinaryTuplePrefixBuilder builder = new BinaryTuplePrefixBuilder(size,
size, stats.dataEndOffset - stats.dataBeginOffset);
+
+ tuple.parse((index, begin, end) -> {
Review Comment:
Same here:
```
for (int i = 0; i < size; i++) {
byte[] valueBytes = tuple.bytesValue(i);
builder.appendBytes(valueBytes);
}
```
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/BinaryTuplePrefixTest.java:
##########
@@ -76,6 +78,101 @@ public void testInternalBufferReallocation() {
assertThat(prefix.intValue(0), is(Integer.MAX_VALUE));
}
+ @Test
+ public void testCreatePrefixFromBinaryTuple1() {
Review Comment:
Please use more meaningful test names, numbers 1 to 5 don't tell me much
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
