Skip to content
Permalink
Browse files
fix bug of missing offset with index query (#866)
fix: #864
Change-Id: Ie83e15f35eb606b8280f8d3e35994a688ee29ca5
  • Loading branch information
javeme committed Feb 28, 2020
1 parent 9b0290c commit f420c03bc641241b69a76126fecc38f9db1900c2
Showing 11 changed files with 113 additions and 66 deletions.
@@ -96,7 +96,7 @@ public Iterator<BackendEntry> query(CassandraSessionPool.Session session,
Query query) {
ExtendableIterator<BackendEntry> rs = new ExtendableIterator<>();

if (query.limit() == 0 && query.limit() != Query.NO_LIMIT) {
if (query.limit() == 0L && !query.nolimit()) {
LOG.debug("Return empty result(limit=0) for query {}", query);
return rs;
}
@@ -159,7 +159,7 @@ protected List<Select> query2Select(String table, Query query) {
}

protected void setPageState(Query query, List<Select> selects) {
if (query.limit() == Query.NO_LIMIT) {
if (query.nolimit()) {
return;
}
for (Select select : selects) {
@@ -26,18 +26,42 @@

public class IdHolderList extends ArrayList<IdHolder> {

private static final IdHolderList EMPTY_P = new IdHolderList(true);
private static final IdHolderList EMPTY_NP = new IdHolderList(false);

private static final long serialVersionUID = -738694176552424990L;

private final boolean paging;
private final boolean needSkipOffset;

public static IdHolderList empty(boolean paging) {
IdHolderList empty = paging ? EMPTY_P : EMPTY_NP;
empty.clear();
return empty;
}

public IdHolderList(boolean paging) {
this(paging, true);
}

public IdHolderList(boolean paging, boolean needSkipOffset) {
this.paging = paging;
this.needSkipOffset = needSkipOffset;
}

public boolean paging() {
return this.paging;
}

public boolean needSkipOffset() {
return this.needSkipOffset;
}

public boolean sameParameters(IdHolderList other) {
return this.paging == other.paging &&
this.needSkipOffset == other.needSkipOffset;
}

@Override
public boolean add(IdHolder holder) {
E.checkArgument(this.paging == holder.paging(),
@@ -62,11 +62,12 @@ protected Function<Query, QueryResults> fetcher() {
return this.fetcher;
}

public void add(List<IdHolder> holders) {
public void add(IdHolderList holders) {
// IdHolderList is results of one index query, the query is flattened
if (!this.parent.paging()) {
for (QueryHolder q : this.queries) {
if (q instanceof IndexQuery) {
((IndexQuery) q).holders.addAll(holders);
((IndexQuery) q).merge(holders);
return;
}
}
@@ -124,7 +125,6 @@ protected PageIterator fetchNext(PageInfo pageInfo, long pageSize) {
return query.iterator(offset - current, pageInfo.page(), pageSize);
}

@SuppressWarnings("unused")
private static Set<Id> limit(Set<Id> ids, Query query) {
long fromIndex = query.offset();
E.checkArgument(fromIndex <= Integer.MAX_VALUE,
@@ -134,11 +134,11 @@ private static Set<Id> limit(Set<Id> ids, Query query) {
if (query.offset() >= ids.size()) {
return ImmutableSet.of();
}
if (query.limit() == Query.NO_LIMIT && query.offset() == 0) {
if (query.nolimit() && query.offset() == 0) {
return ids;
}
long toIndex = query.offset() + query.limit();
if (query.limit() == Query.NO_LIMIT || toIndex > ids.size()) {
long toIndex = query.total();
if (query.nolimit() || toIndex > ids.size()) {
toIndex = ids.size();
}
assert fromIndex < ids.size();
@@ -191,14 +191,13 @@ public PageIterator iterator(int index, String page, long pageSize) {
Query query = this.query.copy();
query.page(page);
// Not set limit to pageSize due to PageEntryIterator.remaining
if (this.query.limit() == Query.NO_LIMIT) {
if (this.query.nolimit()) {
query.limit(pageSize);
}
QueryResults results = fetcher().apply(query);
QueryResults rs = fetcher().apply(query);
// Must iterate all entries before get the next page
return new PageIterator(results.list().iterator(),
results.queries(),
PageInfo.pageState(results.iterator()));
return new PageIterator(rs.list().iterator(), rs.queries(),
PageInfo.pageState(rs.iterator()));
}

@Override
@@ -212,11 +211,21 @@ public int total() {
*/
private class IndexQuery implements QueryHolder {

// Actual is an instance of IdHolderList
private final List<IdHolder> holders;
// An IdHolder each sub-query
private final IdHolderList holders;
// To skip the offset in parent query
private long offsetToSkip;

public IndexQuery(List<IdHolder> holders) {
public IndexQuery(IdHolderList holders) {
this.holders = holders;
this.offsetToSkip = this.holders.needSkipOffset() ?
parent().offset() : -1L;
}

public void merge(IdHolderList holders) {
E.checkState(holders.sameParameters(this.holders),
"Can't merge IdHolderList with different parameters");
this.holders.addAll(holders);
}

@Override
@@ -229,18 +238,23 @@ public QueryResults iterator() {

private QueryResults each(IdHolder holder) {
Set<Id> ids = holder.ids();
if (ids.isEmpty()) {
return null;
}
if (parent().limit() != Query.NO_LIMIT &&
ids.size() > parent().limit()) {
Query parent = parent();
if (this.offsetToSkip > 0L) {
// Skip offset when needed
this.offsetToSkip -= ids.size();
ids = limit(ids, parent);
} else if (!parent.nolimit() && ids.size() > parent.total()) {
/*
* Avoid too many ids in one time query,
* Assume it will get one result by each id
*/
ids = CollectionUtil.subSet(ids, 0, (int) parent().limit());
ids = CollectionUtil.subSet(ids, 0, (int) parent.total());
}

if (ids.isEmpty()) {
return null;
}
IdQuery query = new IdQuery(parent(), ids);
IdQuery query = new IdQuery(parent, ids);
return fetcher().apply(query);
}

@@ -252,8 +266,8 @@ public PageIterator iterator(int index, String page, long pageSize) {
return PageIterator.EMPTY;
}
IdQuery query = new IdQuery(parent(), pageIds.ids());
QueryResults results = fetcher().apply(query);
return new PageIterator(results.iterator(), results.queries(),
QueryResults rs = fetcher().apply(query);
return new PageIterator(rs.iterator(), rs.queries(),
pageIds.pageState());
}

@@ -156,6 +156,10 @@ public void limit(long limit) {
this.limit = limit;
}

public boolean nolimit() {
return this.limit() == NO_LIMIT;
}

public boolean reachLimit(long count) {
long limit = this.limit();
if (limit == NO_LIMIT) {
@@ -178,7 +182,7 @@ public void range(long start, long end) {

// Update limit
if (end != -1L) {
if (this.limit() != Query.NO_LIMIT) {
if (!this.nolimit()) {
end = Math.min(end, offset + this.limit());
} else {
assert end < Query.NO_LIMIT;
@@ -147,8 +147,7 @@ public Iterator<BackendEntry> query(BackendSession session, Query query) {
}
iterator = this.skipOffset(iterator, query.offset());

if (query.limit() != Query.NO_LIMIT &&
query.offset() + query.limit() < rs.size()) {
if (!query.nolimit() && query.total() < rs.size()) {
iterator = this.dropTails(iterator, query.limit());
}
return iterator;
@@ -325,7 +325,7 @@ private boolean existUniqueValueInStore(IndexLabel indexLabel,
* @return converted id query
*/
@Watched(prefix = "index")
public List<IdHolder> queryIndex(ConditionQuery query) {
public IdHolderList queryIndex(ConditionQuery query) {
// Index query must have been flattened in Graph tx
query.checkFlattened();

@@ -355,7 +355,7 @@ public List<IdHolder> queryIndex(ConditionQuery query) {
}

@Watched(prefix = "index")
private List<IdHolder> queryByLabel(ConditionQuery query) {
private IdHolderList queryByLabel(ConditionQuery query) {
HugeType queryType = query.resultType();
IndexLabel il = IndexLabel.label(queryType);
Id label = query.condition(HugeKeys.LABEL);
@@ -390,13 +390,15 @@ private List<IdHolder> queryByLabel(ConditionQuery query) {
indexQuery.capacity(query.capacity());

IdHolder idHolder = this.doIndexQuery(il, indexQuery);
List<IdHolder> holders = new IdHolderList(query.paging());

// NOTE: the backend itself will skip the offset
IdHolderList holders = new IdHolderList(query.paging(), false);
holders.add(idHolder);
return holders;
}

@Watched(prefix = "index")
private List<IdHolder> queryByUserprop(ConditionQuery query) {
private IdHolderList queryByUserprop(ConditionQuery query) {
// Get user applied label or collect all qualified labels with
// related index labels
Set<MatchedIndex> indexes = this.collectMatchedIndexes(query);
@@ -406,12 +408,12 @@ private List<IdHolder> queryByUserprop(ConditionQuery query) {
}

// Value type of Condition not matched
boolean paging = query.paging();
if (!validQueryConditionValues(this.graph(), query)) {
return ImmutableList.of();
return IdHolderList.empty(paging);
}

// Do index query
boolean paging = query.paging();
IdHolderList holders = new IdHolderList(paging);
long idsSize = 0;
for (MatchedIndex index : indexes) {
@@ -430,6 +432,12 @@ private List<IdHolder> queryByUserprop(ConditionQuery query) {
holders.add(holder);
}

/*
* Finish early if records exceeds required.
* NOTE: need to skip the offset if offset > 0, but can't handle
* it here because the query may a sub-query after flatten,
* so the offset will be handle in QueryList.IndexQuery
*/
idsSize += holders.idsSize();
if (query.reachLimit(idsSize)) {
break;
@@ -45,7 +45,7 @@
import com.baidu.hugegraph.backend.id.EdgeId;
import com.baidu.hugegraph.backend.id.Id;
import com.baidu.hugegraph.backend.id.SplicingIdGenerator;
import com.baidu.hugegraph.backend.page.IdHolder;
import com.baidu.hugegraph.backend.page.IdHolderList;
import com.baidu.hugegraph.backend.page.PageInfo;
import com.baidu.hugegraph.backend.page.QueryList;
import com.baidu.hugegraph.backend.query.Condition;
@@ -554,8 +554,7 @@ public Iterator<Vertex> queryVertices() {
}

public Iterator<Vertex> queryVertices(Query query) {
E.checkArgument(this.removedVertices.isEmpty() ||
query.limit() == Query.NO_LIMIT,
E.checkArgument(this.removedVertices.isEmpty() || query.nolimit(),
"It's not allowed to query with limit when " +
"there are uncommitted delete records.");

@@ -701,8 +700,7 @@ public Iterator<Edge> queryEdges() {
}

public Iterator<Edge> queryEdges(Query query) {
E.checkArgument(this.removedEdges.isEmpty() ||
query.limit() == Query.NO_LIMIT,
E.checkArgument(this.removedEdges.isEmpty() || query.nolimit(),
"It's not allowed to query with limit when " +
"there are uncommitted delete records.");

@@ -1104,7 +1102,7 @@ private Query optimizeQuery(ConditionQuery query) {
return null;
}

private List<IdHolder> indexQuery(ConditionQuery query) {
private IdHolderList indexQuery(ConditionQuery query) {
/*
* Optimize by index-query
* It will return a list of id (maybe empty) if success,
@@ -123,7 +123,7 @@ public void eliminate(Session session, BackendEntry entry) {

@Override
public Iterator<BackendEntry> query(Session session, Query query) {
if (query.limit() == 0 && query.limit() != Query.NO_LIMIT) {
if (query.limit() == 0L && !query.nolimit()) {
LOG.debug("Return empty result(limit=0) for query {}", query);
return ImmutableList.<BackendEntry>of().iterator();
}
@@ -296,7 +296,7 @@ public void eliminate(Session session, MysqlBackendEntry.Row entry) {
public Iterator<BackendEntry> query(Session session, Query query) {
ExtendableIterator<BackendEntry> rs = new ExtendableIterator<>();

if (query.limit() == 0 && query.limit() != Query.NO_LIMIT) {
if (query.limit() == 0L && !query.nolimit()) {
LOG.debug("Return empty result(limit=0) for query {}", query);
return rs;
}
@@ -344,7 +344,7 @@ protected List<StringBuilder> query2Select(String table, Query query) {
}
if (query.paging()) {
this.wrapPage(selection, query);
} else if (query.limit() != Query.NO_LIMIT || query.offset() > 0) {
} else if (!query.nolimit() || query.offset() > 0) {
this.wrapOffset(selection, query);
}
}
@@ -512,7 +512,7 @@ protected void wrapPage(StringBuilder select, Query query) {

select.append(this.orderByKeys());

if (query.limit() != Query.NO_LIMIT) {
if (!query.nolimit()) {
// Fetch `limit + 1` rows for judging whether reached the last page
select.append(" limit ");
select.append(query.limit() + 1);
@@ -113,7 +113,7 @@ public void eliminate(Session session, BackendEntry entry) {

@Override
public Iterator<BackendEntry> query(Session session, Query query) {
if (query.limit() == 0 && query.limit() != Query.NO_LIMIT) {
if (query.limit() == 0L && !query.nolimit()) {
LOG.debug("Return empty result(limit=0) for query {}", query);
return ImmutableList.<BackendEntry>of().iterator();
}

0 comments on commit f420c03

Please sign in to comment.