8000 RDBCoveringIterator doesn't work but never created (#17981) · olegrok/arangodb@ee513c8 · GitHub
[go: up one dir, main page]
Skip to content

Commit ee513c8

Browse files
MBkktneunhoefKVS85
authored
RDBCoveringIterator doesn't work but never created (arangodb#17981)
* Fix endless loop * Enable RDBCoveringIterator usage * Add todo about another endless loop * Fix bugs around RDBCoveringIterator. * Take out debugging output. * CHANGELOG. * Change decision as to when to use which iterator. * Revert config change. * CHANGELOG. * Take out unneeded constant. --------- Co-authored-by: Max Neunhoeffer <max@arangodb.com> Co-authored-by: Vadim <vadim@arangodb.com>
1 parent f6a1af2 commit ee513c8

File tree

6 files changed

+37
-7
lines changed

6 files changed

+37
-7
lines changed

‎CHANGELOG

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
devel
22
-----
33

4+
* Activate RDB_CoveringIterator and use it for some geo index queries.
5+
This speeds up and simplifies geo queries with geo index which do not use
6+
GEO_DISTANCE.
7+
48
* Fix bug in hotbackup download/restore to make sure no data is mixed up
59
between servers. This fixes a bug introduced in 3.10.
610

‎arangod/GeoIndex/Index.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
286286
if (!max->isValueType(aql::VALUE_TYPE_STRING)) {
287287
qp.maxDistance = max->getDoubleValue();
288288
} // else assert(max->getStringValue() == "unlimited")
289+
qp.distanceRestricted = true;
289290
break;
290291
}
291292
case aql::NODE_TYPE_OPERATOR_BINARY_GE:
@@ -306,6 +307,7 @@ void Index::handleNode(aql::AstNode const* node, aql::Variable const* ref,
306307
THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_INVALID_GEO_VALUE);
307308
}
308309
qp.minDistance = min->getDoubleValue();
310+
qp.distanceRestricted = true;
309311
break;
310312
}
311313
default:
@@ -329,6 +331,7 @@ void Index::parseCondition(aql::AstNode const* node,
329331
if (params.filterType == geo::FilterType::NONE && params.minDistance == 0 &&
330332
params.maxDistance == 0 && params.maxInclusive) {
331333
params.maxDistance = geo::kRadEps * geo::kEarthRadiusInMeters;
334+
params.distanceRestricted = true;
332335
}
333336
}
334337

‎arangod/GeoIndex/Index.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ struct Index {
7676
Result shape(velocypack::Slice doc, geo::ShapeContainer& shape) const;
7777

7878
/// @brief Parse AQL condition into query parameters
79-
/// Public to allow usage by legacy geo indexes
79+
/// Public to allow usage by legacy geo indexes.
8080
static void parseCondition(aql::AstNode const* node,
8181
aql::Variable const* reference,
8282
geo::QueryParams& params, bool legacy);

‎arangod/RocksDBEngine/RocksDBGeoIndex.cpp

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "GeoIndex/Covering.h"
3333
#include "GeoIndex/Near.h"
3434
#include "Logger/Logger.h"
35+
#include "Logger/LogMacros.h"
3536
#include "RocksDBEngine/RocksDBColumnFamilyManager.h"
3637
#include "RocksDBEngine/RocksDBCommon.h"
3738
#include "RocksDBEngine/RocksDBEngine.h"
@@ -365,7 +366,7 @@ class RDBNearIterator final : public IndexIterator {
365366
if (!_iter->Valid()) { // no more valid keys after this
366367
break;
367368
} else if (cmp->Compare(_iter->key(), bds.end()) > 0) {
368-
continue; // beyond range already
369+
continue; // beyond the range already
369370
} else if (cmp->Compare(bds.start(), _iter->key()) <= 0) {
370371
seek = false; // already in range: min <= key <= max
371372
TRI_ASSERT(cmp->Compare(_iter->key(), bds.end()) <= 0);
@@ -560,7 +561,7 @@ class RDBCoveringIterator final : public IndexIterator {
560561
void performScan() {
561562
rocksdb::Comparator const* cmp = _index->comparator();
562563
// list of sorted intervals to scan
563-
if (_gotIntervals) {
564+
if (!_gotIntervals) {
564565
_scan = _covering.intervals();
565566
_gotIntervals = true;
566567
_scanningInterval = 0;
@@ -577,9 +578,22 @@ class RDBCoveringIterator final : public IndexIterator {
577578
if (_scanningInterval > 0) {
578579
TRI_ASSERT(_scan[_scanningInterval - 1].range_max < it.range_min);
579580
if (!_iter->Valid()) { // no more valid keys after this
581+
// Here is why we actually want to give up here:
582+
// Intervals come from cells, two cells either do not intersect,
583+
// or one is contained in the other, the same holds for the intervals.
584+
// The iterator has an implicit upper bound on the column family, if
585+
// we ever run past this for one interval I, then this means that
586+
// there is nothing of interest in the index past the end of the
587+
// interval I, and we have found everything we need in I.
588+
// However, any later interval J will have a beginning which is
589+
// greater or equal to the beginning of I, therefore nothing new
590+
// can be found from interval J. Therefore:
591+
_scanningInterval = _scan.size();
592+
// Besides, if we would not stop here we would have an endless loop.
580593
break;
581594
} else if (cmp->Compare(_iter->key(), bds.end()) > 0) {
582-
continue; // beyond range already
595+
++_scanningInterval; // Move to the next interval, since we are
596+
continue; // beyond range already
583597
} else if (cmp->Compare(bds.start(), _iter->key()) <= 0) {
584598
seek = false; // already in range: min <= key <= max
585599
TRI_ASSERT(cmp->Compare(_iter->key(), bds.end()) <= 0);
@@ -762,9 +776,10 @@ std::unique_ptr<IndexIterator> RocksDBGeoIndex::iteratorForCondition(
762776
if (!params.sorted &&
763777
(params.filterType == geo::FilterType::CONTAINS ||
764778
params.filterType == geo::FilterType::INTERSECTS) &&
765-
(params.minDistanceRad() < geo::kRadEps &&
766-
params.maxDistanceRad() >
767-
geo::kMaxRadiansBetweenPoints - geo::kRadEps)) {
779+
!params.distanceRestricted) {
780+
LOG_TOPIC("54612", DEBUG, Logger::AQL)
781+
<< "Using RDBCoveringIterator for geo index query: "
782+
<< params.toString();
768783
return std::make_unique<RDBCoveringIterator>(monitor, &_collection, trx,
769784
this, std::move(params));
770785
}
@@ -794,6 +809,9 @@ std::unique_ptr<IndexIterator> RocksDBGeoIndex::iteratorForCondition(
794809
params.cover.bestIndexedLevel = _coverParams.bestIndexedLevel;
795810
}
796811

812+
LOG_TOPIC("54613", DEBUG, Logger::AQL)
813+
<< "Using RDBNearIterator for geo index query: " << params.toString();
814+
797815
if (params.ascending) {
798816
return std::make_unique<RDBNearIterator<geo_index::DocumentsAscending>>(
799817
monitor, &_collection, trx, this, std::move(params));

‎lib/Geo/GeoParams.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ std::string QueryParams::toString() const {
8888
" incl: ", t(minInclusive),
8989
" maxDistance: ", maxDistance,
9090
" incl: ", t(maxInclusive),
91+
" distanceRestricted: ", t(distanceRestricted),
9192
" sorted: ", t(sorted),
9293
" ascending: ", t(ascending),
9394
" origin: ", origin.lng().degrees(), " , ", origin.lat().degrees(),

‎lib/Geo/GeoParams.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,10 @@ struct QueryParams {
121121
double maxDistance = kMaxDistanceBetweenPoints;
122122
bool maxInclusive = false;
123123

124+
/// Some condition on min and max distances are given, this starts out
125+
/// as false and must be set whenever the values minDistance, minInclusive
126+
/// maxDistance and maxInclusive are intended to take effect.
127+
bool distanceRestricted = false;
124128
/// @brief results need to be sorted by distance to centroid
125129
bool sorted = false;
126130
/// @brief Default order is from closest to farthest

0 commit comments

Comments
 (0)
0