You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/12/08 09:13:14 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
tkalkirill opened a new pull request, #1424:
URL: https://github.com/apache/ignite-3/pull/1424
https://issues.apache.org/jira/browse/IGNITE-18243
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill merged pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill merged PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046146547
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -237,6 +238,38 @@ public IndexRow next() {
return new IndexRowImpl(new BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
}
+ @Override
+ public @Nullable IndexRow peek() {
+ if (hasNext != null && !hasNext) {
Review Comment:
Yes, they are similar, `advanceIfNeeded` can change the state of both `TestSortedIndexStorage.ScanCursor#indexMapEntry` and `TestSortedIndexStorage.ScanCursor#row`, which makes it a little difficult to highlight the common parts.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java:
##########
@@ -337,6 +338,31 @@ public IndexRow next() {
}
}
+ @Override
+ public @Nullable IndexRow peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ try {
+ SortedIndexRow nextTreeRow;
+
+ if (treeRow == null) {
+ nextTreeRow = lower == null ? sortedIndexTree.findFirst() : sortedIndexTree.findNext(lower, true);
Review Comment:
Same here
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046784677
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -237,6 +238,42 @@ public IndexRow next() {
return new IndexRowImpl(new BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
}
+ @Override
+ public @Nullable IndexRow peek() {
+ if (hasNext != null) {
+ if (hasNext) {
+ return new IndexRowImpl(new BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
+ }
+
+ return null;
+ }
+
+ Entry<ByteBuffer, NavigableMap<RowId, Object>> indexMapEntry0 = indexMapEntry == null ? indexMap.firstEntry() : indexMapEntry;
Review Comment:
Can you please rename `indexMapEntry` to something like `currentEntry`?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046148067
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
Review Comment:
fix it
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
Review Comment:
why?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046717549
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
Review Comment:
I think that the basic iterator/cursor usage pattern is:
`
while(cursor.hasNext()){
handle(cursor.next());
}
`
If `hasNext()` returns `false`, then we exit the loop assuming that we have finished processing the range.
Therefore, it seems that further calls to `hasNext()`, `next()` and `peek()` should in my opinion be interpreted as nothing more to return.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046873770
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
Review Comment:
Fix it
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046783412
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
Review Comment:
ok, makes sense
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046767594
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
Review Comment:
Because you can write this code without it (it's just a nitpick, no big deal):
```
if (!it.isValid()) {
RocksUtils.checkIterator(it);
return null;
} else {
return mapper.apply(ByteBuffer.wrap(it.key()).order(ORDER));
}
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046161193
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ no cached row
+ // cursor1 = ^ no cached row
+ scan0 = indexStorage.scan(null, null, 0);
+ scan1 = indexStorage.scan(null, null, 0);
+
+ assertEquals(0, serializer.deserializeColumns(scan0.peek())[0]);
+ assertEquals(0, serializer.deserializeColumns(scan1.peek())[0]);
+
+ // index = [0]
+ // cursor0 = ^ cached [0]
+ assertTrue(scan0.hasNext());
Review Comment:
I will fix
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046875028
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -237,6 +238,42 @@ public IndexRow next() {
return new IndexRowImpl(new BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
}
+ @Override
+ public @Nullable IndexRow peek() {
+ if (hasNext != null) {
+ if (hasNext) {
+ return new IndexRowImpl(new BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
+ }
+
+ return null;
+ }
+
+ Entry<ByteBuffer, NavigableMap<RowId, Object>> indexMapEntry0 = indexMapEntry == null ? indexMap.firstEntry() : indexMapEntry;
Review Comment:
Fix it
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046147005
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
+
+ refreshAndPrepareRocksIterator();
+
+ if (!it.isValid()) {
+ RocksUtils.checkIterator(it);
+ } else {
+ next = mapper.apply(ByteBuffer.wrap(it.key()).order(ORDER));
+ }
+
+ return next;
}
private void advanceIfNeeded() throws StorageException {
if (hasNext != null) {
return;
}
+ refreshAndPrepareRocksIterator();
Review Comment:
Yep
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1045867457
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
+
+ refreshAndPrepareRocksIterator();
+
+ if (!it.isValid()) {
+ RocksUtils.checkIterator(it);
+ } else {
+ next = mapper.apply(ByteBuffer.wrap(it.key()).order(ORDER));
+ }
+
+ return next;
}
private void advanceIfNeeded() throws StorageException {
if (hasNext != null) {
return;
}
+ refreshAndPrepareRocksIterator();
Review Comment:
We will now refresh the iterator on every `next` call as well, is that intended?
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
Review Comment:
Shouldn't these cursor start returning 0, after it was inserted?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java:
##########
@@ -337,6 +338,31 @@ public IndexRow next() {
}
}
+ @Override
+ public @Nullable IndexRow peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ try {
+ SortedIndexRow nextTreeRow;
+
+ if (treeRow == null) {
+ nextTreeRow = lower == null ? sortedIndexTree.findFirst() : sortedIndexTree.findNext(lower, true);
Review Comment:
Same here
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ no cached row
+ // cursor1 = ^ no cached row
+ scan0 = indexStorage.scan(null, null, 0);
+ scan1 = indexStorage.scan(null, null, 0);
+
+ assertEquals(0, serializer.deserializeColumns(scan0.peek())[0]);
+ assertEquals(0, serializer.deserializeColumns(scan1.peek())[0]);
+
+ // index = [0]
+ // cursor0 = ^ cached [0]
+ assertTrue(scan0.hasNext());
Review Comment:
This is very strange: why does calling `hasNext` invalidate what `peek` returns? I thought only `next` can do that
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -237,6 +238,38 @@ public IndexRow next() {
return new IndexRowImpl(new BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
}
+ @Override
+ public @Nullable IndexRow peek() {
+ if (hasNext != null && !hasNext) {
Review Comment:
This method is very similar to `advanceIfNeeded`, can we extract the common code?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -202,14 +208,47 @@ public ByteBuffer next() {
this.hasNext = null;
- return ByteBuffer.wrap(key).order(ORDER);
+ return mapper.apply(ByteBuffer.wrap(key).order(ORDER));
+ }
+
+ @Override
+ public @Nullable T peek() {
+ if (hasNext != null && !hasNext) {
+ return null;
+ }
+
+ T next = null;
Review Comment:
This variable is redundant
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046178327
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
Review Comment:
What do you mean by "cursor has finished its work"? Especially if new underlying data can appear at any time? I would say that cursor should stop returning any data only if it is closed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1424: IGNITE-18243 Implement a peek method for the sorted index cursor
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1424:
URL: https://github.com/apache/ignite-3/pull/1424#discussion_r1046169505
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/AbstractSortedIndexStorageTest.java:
##########
@@ -1233,6 +1235,276 @@ void testScanContractRemoveNextAndAddFirstRow() {
assertThrows(NoSuchElementException.class, scan::next);
}
+
+ @Test
+ void testScanPeekForFinishedCursor() {
+ SortedIndexDefinition indexDefinition = SchemaBuilders.sortedIndex("TEST_IDX")
+ .addIndexColumn(ColumnType.INT32.typeSpec().name()).asc().done()
+ .build();
+
+ SortedIndexStorage indexStorage = createIndexStorage(indexDefinition);
+
+ BinaryTupleRowSerializer serializer = new BinaryTupleRowSerializer(indexStorage.indexDescriptor());
+
+ PeekCursor<IndexRow> scan0 = indexStorage.scan(null, null, 0);
+ PeekCursor<IndexRow> scan1 = indexStorage.scan(null, null, 0);
+
+ // index =
+ // cursor0 = ^ already finished
+ assertFalse(scan0.hasNext());
+ assertNull(scan0.peek());
+
+ // index =
+ // cursor1 = ^ already finished
+ assertThrows(NoSuchElementException.class, scan1::next);
+ assertNull(scan1.peek());
+
+ // index = [0]
+ // cursor0 = ^ already finished
+ // cursor1 = ^ already finished
+ put(indexStorage, serializer.serializeRow(new Object[]{0}, new RowId(TEST_PARTITION)));
+
+ assertNull(scan0.peek());
Review Comment:
I don't think it's necessary, because the cursor has finished its work.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org