rpuch commented on code in PR #1407:
URL: https://github.com/apache/ignite-3/pull/1407#discussion_r1044160775
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -208,4 +184,91 @@ private void checkClosed() {
throw new StorageClosedException("Storage is already closed");
}
}
+
+ private class ScanCursor implements Cursor<IndexRow> {
+ private final NavigableMap<ByteBuffer, NavigableMap<RowId, Object>>
indexMap;
+
+ @Nullable
+ private Boolean hasNext;
+
+ @Nullable
+ private Entry<ByteBuffer, NavigableMap<RowId, Object>> indexMapEntry;
+
+ @Nullable
+ private RowId rowId;
+
+ private ScanCursor(NavigableMap<ByteBuffer, NavigableMap<RowId,
Object>> indexMap) {
+ this.indexMap = indexMap;
+ }
+
+ @Override
+ public void close() {
+ // No-op.
+ }
+
+ @Override
+ public boolean hasNext() {
+ checkClosed();
+
+ advanceIfNeeded();
+
+ return hasNext;
+ }
+
+ @Override
+ public IndexRow next() {
+ checkClosed();
+
+ advanceIfNeeded();
+
+ boolean hasNext = this.hasNext;
+
+ this.hasNext = null;
+
+ if (!hasNext) {
+ throw new NoSuchElementException();
+ }
+
+ return new IndexRowImpl(new
BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
+ }
+
+ private void advanceIfNeeded() {
+ if (hasNext == null) {
Review Comment:
How about inverting the `if` so that we exit the method if `hasNext` is not
`null`, so that the principal logic of the method has one indentation level
less?
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -208,4 +184,91 @@ private void checkClosed() {
throw new StorageClosedException("Storage is already closed");
}
}
+
+ private class ScanCursor implements Cursor<IndexRow> {
+ private final NavigableMap<ByteBuffer, NavigableMap<RowId, Object>>
indexMap;
+
+ @Nullable
+ private Boolean hasNext;
+
+ @Nullable
+ private Entry<ByteBuffer, NavigableMap<RowId, Object>> indexMapEntry;
+
+ @Nullable
+ private RowId rowId;
+
+ private ScanCursor(NavigableMap<ByteBuffer, NavigableMap<RowId,
Object>> indexMap) {
+ this.indexMap = indexMap;
+ }
+
+ @Override
+ public void close() {
+ // No-op.
+ }
+
+ @Override
+ public boolean hasNext() {
+ checkClosed();
+
+ advanceIfNeeded();
+
+ return hasNext;
+ }
+
+ @Override
+ public IndexRow next() {
+ checkClosed();
+
+ advanceIfNeeded();
+
+ boolean hasNext = this.hasNext;
+
+ this.hasNext = null;
+
+ if (!hasNext) {
+ throw new NoSuchElementException();
+ }
+
+ return new IndexRowImpl(new
BinaryTuple(descriptor.binaryTupleSchema(), indexMapEntry.getKey()), rowId);
+ }
+
+ private void advanceIfNeeded() {
+ if (hasNext == null) {
+ if (indexMapEntry == null) {
+ indexMapEntry = indexMap.firstEntry();
+ }
+
+ if (rowId == null) {
+ if (indexMapEntry != null) {
+ rowId =
getRowId(indexMapEntry.getValue().firstEntry());
+ }
+ } else {
+ Entry<RowId, Object> rowIdEntry =
indexMapEntry.getValue().higherEntry(rowId);
Review Comment:
Should this be named `nextRowIdEntry`?
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -167,24 +167,71 @@ private Cursor<ByteBuffer> createScanCursor(byte
@Nullable [] lowerBound, byte @
RocksIterator it = indexCf.newIterator(options);
- if (lowerBound == null) {
- it.seek(partitionStorage.partitionStartPrefix());
- } else {
- it.seek(lowerBound);
- }
+ return new Cursor<>() {
+ @Nullable
+ private Boolean hasNext;
- return new RocksIteratorAdapter<>(it) {
- @Override
- protected ByteBuffer decodeEntry(byte[] key, byte[] value) {
- return ByteBuffer.wrap(key).order(ORDER);
- }
+ @Nullable
+ private byte[] key;
Review Comment:
```suggestion
private byte @Nullable [] key;
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java:
##########
@@ -272,4 +271,97 @@ public R next() {
}
};
}
+
+ private class ScanCursor implements Cursor<IndexRow> {
+ @Nullable
+ private Boolean hasNext;
+
+ @Nullable
+ private final SortedIndexRowKey lower;
+
+ @Nullable
+ private final SortedIndexRowKey upper;
+
+ @Nullable
+ private SortedIndexRow treeRow;
+
+ private ScanCursor(@Nullable SortedIndexRowKey lower, @Nullable
SortedIndexRowKey upper) {
+ this.lower = lower;
+ this.upper = upper;
+ }
+
+ @Override
+ public void close() {
+ // No-op.
+ }
+
+ @Override
+ public boolean hasNext() {
+ if (!closeBusyLock.enterBusy()) {
+ throwStorageClosedException();
+ }
+
+ try {
+ advanceIfNeeded();
+
+ return hasNext;
+ } catch (IgniteInternalCheckedException e) {
+ throw new StorageException("Error while advancing the cursor",
e);
+ } finally {
+ closeBusyLock.leaveBusy();
+ }
+ }
+
+ @Override
+ public IndexRow next() {
+ if (!closeBusyLock.enterBusy()) {
+ throwStorageClosedException();
+ }
+
+ try {
+ advanceIfNeeded();
+
+ boolean hasNext = this.hasNext;
+
+ this.hasNext = null;
+
+ if (!hasNext) {
+ throw new NoSuchElementException();
+ }
+
+ return toIndexRowImpl(treeRow);
+ } catch (IgniteInternalCheckedException e) {
+ throw new StorageException("Error while advancing the cursor",
e);
+ } finally {
+ closeBusyLock.leaveBusy();
+ }
+ }
+
+ private void advanceIfNeeded() throws IgniteInternalCheckedException {
+ if (hasNext == null) {
Review Comment:
Early return pattern could be used to decrease indentation level for the
principal logic of the method
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/RocksDbSortedIndexStorage.java:
##########
@@ -167,24 +167,71 @@ private Cursor<ByteBuffer> createScanCursor(byte
@Nullable [] lowerBound, byte @
RocksIterator it = indexCf.newIterator(options);
- if (lowerBound == null) {
- it.seek(partitionStorage.partitionStartPrefix());
- } else {
- it.seek(lowerBound);
- }
+ return new Cursor<>() {
+ @Nullable
+ private Boolean hasNext;
- return new RocksIteratorAdapter<>(it) {
- @Override
- protected ByteBuffer decodeEntry(byte[] key, byte[] value) {
- return ByteBuffer.wrap(key).order(ORDER);
- }
+ @Nullable
+ private byte[] key;
@Override
public void close() {
- super.close();
+ it.close();
Review Comment:
Let's include `it` into the call to `closeAll()`
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/index/impl/TestSortedIndexStorage.java:
##########
@@ -45,7 +44,9 @@
* Test implementation of MV sorted index storage.
*/
public class TestSortedIndexStorage implements SortedIndexStorage {
- private final ConcurrentNavigableMap<ByteBuffer, Set<RowId>> index;
+ private static final Object NULL = new Object();
+
+ private final ConcurrentNavigableMap<ByteBuffer, NavigableMap<RowId,
Object>> index;
Review Comment:
It looks like this can still be `ConcurrentNavigableMap<ByteBuffer,
NavigableSet<RowId>>` which is simpler to use and it causes less confusion
(because something that is used as a set is called Set, not Map)
--
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]
