imbajin commented on code in PR #2994: URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3467750863
########## hugegraph-server/hugegraph-hstore/src/test/java/org/apache/hugegraph/backend/store/hstore/HstoreTableTest.java: ########## @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package org.apache.hugegraph.backend.store.hstore; + +import java.util.Arrays; +import java.util.List; +import java.util.NoSuchElementException; + +import org.apache.hugegraph.backend.id.Id.IdType; +import org.apache.hugegraph.backend.id.IdGenerator; +import org.apache.hugegraph.backend.page.PageInfo; +import org.apache.hugegraph.backend.page.PageState; +import org.apache.hugegraph.backend.query.IdRangeQuery; +import org.apache.hugegraph.backend.query.Query; +import org.apache.hugegraph.backend.store.BackendEntry; +import org.apache.hugegraph.backend.store.BackendEntry.BackendColumn; +import org.apache.hugegraph.backend.store.BackendEntry.BackendColumnIterator; +import org.apache.hugegraph.backend.store.BackendEntryIterator; +import org.apache.hugegraph.type.HugeType; +import org.junit.Assert; +import org.junit.Test; +import org.mockito.Mockito; + +public class HstoreTableTest { + + @Test + public void testRangeIndexPageStateComesFromBackendIteratorPosition() { + Query query = new Query(HugeType.RANGE_INT_INDEX); + query.page(""); + query.limit(1L); + + BackendEntryIterator iterator = HstoreTable.newEntryIterator( + new TestColumnIterator(1, 2), query); + + Assert.assertTrue(iterator.hasNext()); + BackendEntry entry = iterator.next(); + Assert.assertArrayEquals(keyBytes(1), entry.id().asBytes()); + + PageState pageState = PageInfo.pageState(iterator); + Assert.assertArrayEquals(keyBytes(2), pageState.position()); Review Comment: ‼️ **Do not use a prefetched row as the next page token** Evidence: this test returns one entry (`keyBytes(1)`) and then asserts the page token is `keyBytes(2)`, which is the next row already prefetched by the iterator but not returned to the caller. Impact: the next page can resume after that prefetched-but-undelivered row and drop it; this matches the current hstore failure shape where paged property results expect 5 rows but get 4. Please make the page token represent the last emitted row, or make resume semantics include the prefetched row, and add an HStore paging regression that fetches page 1, resumes with the returned token, and proves no row is skipped or duplicated. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphIndexTransaction.java: ########## @@ -724,8 +731,11 @@ private PageIds doIndexQueryOnce(IndexLabel indexLabel, Set<Id> ids = InsertionOrderUtil.newSet(); entries = super.query(query).iterator(); while (entries.hasNext()) { - HugeIndex index = this.serializer.readIndex(graph(), query, - entries.next()); + HugeIndex index = this.readMatchedIndex(indexLabel, query, + entries.next()); + if (index == null) { + continue; Review Comment: ‼️ **Keep paging scans advancing past skipped index rows** Evidence: this new skip path can leave `ids` empty even when the backend iterator has advanced to a non-terminal page. `doIndexQueryOnce()` then returns `PageIds.EMPTY` when `ids.isEmpty()`, and `PagingIdHolder.fetchNext()` marks the holder exhausted whenever `result.ids().size() < pageSize`, so a page containing only stale or mismatched index entries stops the whole paged query. Impact: paged index scans can miss valid ids that live on later backend pages, especially on HStore paths where stale or mismatched rows are exactly what `readMatchedIndex()` is trying to tolerate. Please keep scanning until either a valid id is collected or the backend page state is exhausted, and add coverage where the first backend page is fully skipped but the next page contains a valid index entry. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
