xtern commented on code in PR #2728:
URL: https://github.com/apache/ignite-3/pull/2728#discussion_r1368923816
##########
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
Review Comment:
This flag doesn't look useful.
```
BinaryTupleBuilder(int numElements, int totalValueSize)
BinaryTupleBuilder(int numElements, int totalValueSize, boolean
doNotIgnoreTotalValueSize)
```
looks strange to me :thinking:
I suggest to rework related code in `SqlRowHandler#toBinaryTuple` and revert
changes here
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SqlRowHandler.java:
##########
@@ -55,12 +55,11 @@
* </ul>
*
* <p>Each kind of rows is serialized to the same binary tuple format
- * using the {@link #toByteBuffer(RowWrapper) toByteBuffer} method.
+ * using the {@link #toBinaryTuple(RowWrapper)} method.
*
- * <p>Factory methods {@link RowFactory#create(InternalTuple)
wrap(InternalTuple)} and
- * {@link RowFactory#create(ByteBuffer) create(ByteBuffer)} allow create rows
without
- * any additional conversions. But the fields in binary tuple must match the
- * factory {@link RowSchema row schema}.
+ * <p>Factory method {@link RowFactory#create(InternalTuple)
wrap(InternalTuple)}
+ * allow create rows without any additional conversions. But the fields in
binary
+ * tuple must match the factory {@link RowSchema row schema}.
Review Comment:
please also fix this description :flushed:
```suggestion
* <p>Factory method {@link RowFactory#create(InternalTuple)} allows
* creating rows without any additional conversions. But the fields
* in binary tuple must match the factory {@link RowSchema row schema}.
```
##########
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:
I suggest using the existing BinaryTupleBuilder constructors instead of
adding flag `exactEstimate`.
```suggestion
BinaryTupleBuilder tupleBuilder = exactEstimate
? new BinaryTupleBuilder(row.length, estimatedSize)
: new BinaryTupleBuilder(row.length);
```
--
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]
