korlov42 commented on code in PR #2728:
URL: https://github.com/apache/ignite-3/pull/2728#discussion_r1368362341
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ScannableTableImpl.java:
##########
@@ -148,24 +147,31 @@ public <RowT> Publisher<RowT> indexLookup(
PartitionWithTerm partWithTerm,
RowFactory<RowT> rowFactory,
int indexId,
- List<String> columns, RowT key,
+ List<String> columns,
+ RowT key,
@Nullable BitSet requiredColumns
) {
-
- BinaryTupleSchema indexRowSchema =
RowConverter.createIndexRowSchema(columns, tableDescriptor);
TxAttributes txAttributes = ctx.txAttributes();
+ RowHandler<RowT> handler = rowFactory.handler();
Publisher<BinaryRow> pub;
- BinaryTuple keyTuple = toBinaryTuple(ctx, indexRowSchema, key,
rowFactory);
+ BinaryTuple keyTuple = handler.toBinaryTuple(key);
+
+ assert keyTuple.elementCount() == columns.size()
+ : format("Key should contain exactly {} fields, but was {}",
columns.size(), handler.toString(key));
if (txAttributes.readOnly()) {
+ HybridTimestamp readTime = txAttributes.time();
+
+ assert readTime != null;
+
pub = internalTable.lookup(
partWithTerm.partId(),
- txAttributes.time(),
+ readTime,
ctx.localNode(),
indexId,
keyTuple,
- requiredColumns
+ null
Review Comment:
> do we need them ?
I don't know 🤷♂️ It's part of internal table API which is not implemented.
From sql point of view it's safer to pass 'null' explicitly, otherwise we will
have problems if someone will decide to implement missing functionality
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ScannableTableImpl.java:
##########
@@ -148,24 +147,31 @@ public <RowT> Publisher<RowT> indexLookup(
PartitionWithTerm partWithTerm,
RowFactory<RowT> rowFactory,
int indexId,
- List<String> columns, RowT key,
+ List<String> columns,
+ RowT key,
@Nullable BitSet requiredColumns
) {
-
- BinaryTupleSchema indexRowSchema =
RowConverter.createIndexRowSchema(columns, tableDescriptor);
TxAttributes txAttributes = ctx.txAttributes();
+ RowHandler<RowT> handler = rowFactory.handler();
Publisher<BinaryRow> pub;
- BinaryTuple keyTuple = toBinaryTuple(ctx, indexRowSchema, key,
rowFactory);
+ BinaryTuple keyTuple = handler.toBinaryTuple(key);
+
+ assert keyTuple.elementCount() == columns.size()
+ : format("Key should contain exactly {} fields, but was {}",
columns.size(), handler.toString(key));
if (txAttributes.readOnly()) {
+ HybridTimestamp readTime = txAttributes.time();
+
+ assert readTime != null;
+
pub = internalTable.lookup(
partWithTerm.partId(),
- txAttributes.time(),
+ readTime,
ctx.localNode(),
indexId,
keyTuple,
- requiredColumns
+ null
Review Comment:
> do we need them ?
I don't know 🤷♂️ It's part of internal table API which is not implemented.
From sql point of view it's safer to pass 'null' explicitly, otherwise we will
have problems if someone will decide to implement missing functionality
--
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]
