korlov42 commented on code in PR #2728:
URL: https://github.com/apache/ignite-3/pull/2728#discussion_r1369698209


##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SqlRowHandler.java:
##########
@@ -226,24 +219,61 @@ void set(int field, @Nullable Object value) {
         }
 
         @Override
-        ByteBuffer toByteBuffer() {
-            BinaryTupleBuilder tupleBuilder = new 
BinaryTupleBuilder(row.length);
+        BinaryTuple toBinaryTuple() {
+            int estimatedSize = 0;
+            boolean exactEstimate = true;
+            for (int i = 0; i < row.length; i++) {
+                NativeType nativeType = 
RowSchemaTypes.toNativeType(rowSchema.fields().get(i));
+
+                if (nativeType == null) {
+                    assert row[i] == null;
+
+                    continue;
+                }
+
+                Object value = row[i];
+
+                if (value == null) {
+                    continue;
+                }
+
+                if (nativeType.spec().fixedLength()) {
+                    estimatedSize += nativeType.sizeInBytes();
+                } else {
+                    if (value instanceof String) {
+                        // every character in the string may contain up to 4 
bytes.
+                        // Let's be optimistic here and reserve buffer only 
for the smallest
+                        // possible variant
+
+                        estimatedSize += ((String) value).length();
+                        exactEstimate = false;
+                    } else if (value instanceof ByteString) {
+                        estimatedSize += ((ByteString) value).length();
+                    } else {
+                        assert (value instanceof BigDecimal) || (value 
instanceof BigInteger) : "unexpected value " + value.getClass();
+
+                        exactEstimate = false;
+                    }
+                }
+            }
+
+            BinaryTupleBuilder tupleBuilder = new 
BinaryTupleBuilder(row.length, estimatedSize, exactEstimate);

Review Comment:
   whole point of this patch is to avoid using this `new 
BinaryTupleBuilder(row.length)` version of constructor. The problem here is 
without any estimate it preallocates buffer with size ~4kb (see 
`BinaryTupleBuilder#DEFAULT_BUFFER_SIZE`).
   
   If we will follow your suggestion, we fix issue for all types but string, 
decimal, and number. I'm not sure if anyone is indexing decimals, but I 
definitely sure string is a popular choice for PK. Thus, I would sacrifice a 
little code clarity in favour of performance



-- 
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]

Reply via email to