8000 Bug fix/cursor write compact results (#13620) · arangodb/arangodb@8a13c88 · GitHub
[go: up one dir, main page]
Skip to content

Commit 8a13c88

Browse files
goedderzjsteemann
goedderz
and
jsteemann
authored
Bug fix/cursor write compact results (#13620)
Co-authored-by: jsteemann <jan@arangodb.com>
1 parent a7ad000 commit 8a13c88

File tree

8 files changed

+262
-16
lines changed

8 files changed

+262
-16
lines changed

‎3rdParty/velocypack/include/velocypack/Builder.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,11 @@ class Builder {
418418
return addInternal<Slice>(sub);
419419
}
420420

421+
// Add a shared slice to an array
422+
inline uint8_t* add(SharedSlice const& sub) {
423+
return addInternal<Slice>(sub.slice());
424+
}
425+
421426
// Add a subvalue into an array from a ValuePair:
422427
inline uint8_t* add(ValuePair const& sub) {
423428
return addInternal<ValuePair>(sub);

‎CHANGELOG‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ devel
7474
Instead of offering `flexible` and `single`, now use the more intuitive
7575
`Sharded` and `OneShard` options, and update the help text for them.
7676

77+
* Make all AQL cursors return compact result arrays.
78+
7779
* Fix profiling of AQL queries with the `silent` and `stream` options sets in
7880
combination. Using the `silent` option makes a query execute, but discard all
7981
its results instantly. This led to some confusion in streaming queries, which

‎arangod/Aql/QueryCursor.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ Result QueryResultCursor::dumpSync(VPackBuilder& builder) {
101101
// some reallocs
102102
// (not accurate, but the actual size is unknown anyway)
103103
builder.reserve(std::max<size_t>(1, std::min<size_t>(n, 10000)) * 32);
104-
builder.add("result", VPackValue(VPackValueType::Array));
104+
builder.add("result", VPackValue(VPackValueType::Array, true));
105105
for (size_t i = 0; i < n; ++i) {
106106
if (!hasNext()) {
107107
break;
@@ -328,7 +328,7 @@ ExecutionState QueryStreamCursor::writeResult(VPackBuilder& builder) {
328328
if (!silent) {
329329
builder.reserve(16 * 1024);
330330
}
331-
331+
332332
builder.add("result", VPackValue(VPackValueType::Array, true));
333333

334334
aql::ExecutionEngine* engine = _query->rootEngine();

‎arangod/CMakeLists.txt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -975,10 +975,11 @@ target_link_libraries(arango_network llhttp)
975975
target_link_libraries(arango_network nghttp2)
976976
target_link_libraries(arango_network arango::validation)
977977

978+
target_link_libraries(arango_pregel arango)
978979
target_link_libraries(arango_pregel arango_agency)
980+
target_link_libraries(arango_pregel arango_greenspun)
979981
target_link_libraries(arango_pregel boost_boost)
980982
target_link_libraries(arango_pregel boost_system)
981-
target_link_libraries(arango_pregel arango_greenspun)
982983

983984
target_link_libraries(arango_replication arango_storage_engine)
984985
target_link_libraries(arango_replication arango_utils)

‎tests/Aql/QueryCursorTest.cpp‎

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,220 @@
1+
////////////////////////////////////////////////////////////////////////////////
2+
/// DISCLAIMER
3+
///
4+
/// Copyright 2014-2021 ArangoDB GmbH, Cologne, Germany
5+
/// Copyright 2004-2014 triAGENS GmbH, Cologne, Germany
6+
///
7+
/// Licensed under the Apache License, Version 2.0 (the "License");
8+
/// you may not use this file except in compliance with the License.
9+
/// You may obtain a copy of the License at
10+
///
11+
/// http://www.apache.org/licenses/LICENSE-2.0
12+
///
13+
/// Unless required by applicable law or agreed to in writing, software
14+
/// distributed under the License is distributed on an "AS IS" BASIS,
15+
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
/// See the License for the specific language governing permissions and
17+
/// limitations under the License.
18+
///
19+
/// Copyright holder is ArangoDB GmbH, Cologne, Germany
20+
///
21+
/// @author Tobias Gödderz
22+
////////////////////////////////////////////////////////////////////////////////
23+
24+
#include "gtest/gtest.h"
25+
26+
#include "RestHandler/RestCursorHandler.h"
27+
#include "RestServer/QueryRegistryFeature.h"
28+
29+
#include "IResearch/RestHandlerMock.h"
30+
#include "Mocks/Servers.h"
31+
32+
#include <velocypack/Builder.h>
33+
#include <velocypack/Options.h>
34+
#include <velocypack/Parser.h&g C0A6 t;
35+
#include <velocypack/SharedSlice.h>
36+
#include <velocypack/velocypack-aliases.h>
37+
38+
using namespace arangodb::tests;
39+
40+
class QueryCursorTest : public ::testing::Test {
41+
public:
42+
static void SetUpTestCase() {
43+
server = std::make_unique<mocks::MockRestAqlServer>();
44+
}
45+
static void TearDownTestCase() { server.reset(); }
46+
47+
protected:
48+
static inline std::unique_ptr<mocks::MockRestAqlServer> server;
49+
};
50+
51+
namespace {
52+
arangodb::velocypack::SharedSlice operator"" _vpack(const char* json, size_t len) {
53+
VPackOptions options;
54+
options.checkAttributeUniqueness = true;
55+
options.validateUtf8Strings = true;
56+
VPackParser parser(&options);
57+
parser.parse(json, len);
58+
return parser.steal()->sharedSlice();
59+
}
60+
} // namespace
61+
62+
TEST_F(QueryCursorTest, resultCursorResultArrayIndexSingleBatch) {
63+
auto& vocbase = server->getSystemDatabase();
64+
auto fakeRequest = std::make_unique<GeneralRequestMock>(vocbase);
65+
auto fakeResponse = std::make_unique<GeneralResponseMock>();
66+
fakeRequest->setRequestType(arangodb::rest::RequestType::POST);
67+
fakeRequest->_payload.add(R"json(
68+
{
69+
"query": "FOR i IN 1..1000 RETURN CONCAT('', i)"
70+
}
71+
)json"_vpack);
72+
73+
auto* registry = arangodb::QueryRegistryFeature::registry();
74+
75+
auto testee =
76+
std::make_shared<arangodb::RestCursorHandler>(server->server(),
77+
fakeRequest.release(),
78+
fakeResponse.release(), registry);
79+
80+
testee->execute();
81+
82+
fakeResponse.reset(dynamic_cast<GeneralResponseMock*>(testee->stealResponse().release()));
83+
84+
auto const responseBodySlice = fakeResponse->_payload.slice();
85+
86+
ASSERT_TRUE(responseBodySlice.isObject());
87+
88+
auto resultSlice = responseBodySlice.get("result").resolveExternal();
89+
ASSERT_FALSE(resultSlice.isNone());
90+
ASSERT_TRUE(resultSlice.isArray())
91+
<< "Expected array, but got " << valueTypeName(resultSlice.type());
92+
// should be a compact array
93+
ASSERT_EQ(0x13, resultSlice.head());
94+
}
95+
96+
TEST_F(QueryCursorTest, resultCursorResultArrayIndexTwoBatches) {
97+
auto& vocbase = server->getSystemDatabase();
98+
auto fakeRequest = std::make_unique<GeneralRequestMock>(vocbase);
99+
auto fakeResponse = std::make_unique<GeneralResponseMock>();
100+
fakeRequest->setRequestType(arangodb::rest::RequestType::POST);
101+
fakeRequest->_payload.add(R"json(
102+
{
103+
"query": "FOR i IN 1..2000 RETURN CONCAT('', i)"
104+
}
105+
)json"_vpack);
106+
107+
auto* registry = arangodb::QueryRegistryFeature::registry();
108+
109+
auto testee =
110+
std::make_shared<arangodb::RestCursorHandler>(server->server(),
111+
fakeRequest.release(),
112+
fakeResponse.release(), registry);
113+
114+
testee->execute();
115+
116+
fakeResponse.reset(dynamic_cast<GeneralResponseMock*>(testee->stealResponse().release()));
117+
118+
auto const responseBodySlice = fakeResponse->_payload.slice();
119+
120+
ASSERT_TRUE(responseBodySlice.isObject());
121+
122+
auto resultSlice = responseBodySlice.get("result").resolveExternal();
123+
ASSERT_FALSE(resultSlice.isNone());
124+
ASSERT_TRUE(resultSlice.isArray())
125+
<< "Expected array, but got " << valueTypeName(resultSlice.type());
126+
// should be a compact array
127+
ASSERT_EQ(0x13, resultSlice.head());
128+
}
129+
130+
TEST_F(QueryCursorTest, streamingCursorResultArrayIndexSingleBatch) {
131+
auto& vocbase = server->getSystemDatabase();
132+
auto fakeRequest = std::make_unique<GeneralRequestMock>(vocbase);
133+
auto fakeResponse = std::make_unique<GeneralResponseMock>();
134+
fakeRequest->setRequestType(arangodb::rest::RequestType::POST);
135+
fakeRequest->_payload.add(R"json(
136+
{
137+
"query": "FOR i IN 1..1000 RETURN CONCAT('', i)",
138+
"options": { "stream": true }
139+
}
140+
)json"_vpack);
141+
142+
auto* registry = arangodb::QueryRegistryFeature::registry();
143+
144+
auto testee =
145+
std::make_shared<arangodb::RestCursorHandler>(server->server(),
146+
fakeRequest.release(),
147+
fakeResponse.release(), registry);
148+
149+
testee->execute();
150+
151+
fakeResponse.reset(dynamic_cast<GeneralResponseMock*>(testee->stealResponse().release()));
152+
153+
auto const responseBodySlice = fakeResponse->_payload.slice();
154+
155+
ASSERT_TRUE(responseBodySlice.isObject());
156+
157+
auto const resultSlice = responseBodySlice.get("result").resolveExternal();
158+
ASSERT_FALSE(resultSlice.isNone());
159+
ASSERT_TRUE(resultSlice.isArray())
160+
<< "Expected array, but got " << valueTypeName(resultSlice.type());
161+
// should be a compact array
162+
ASSERT_EQ(0x13, resultSlice.head());
163+
}
164+
165+
TEST_F(QueryCursorTest, streamingCursorResultArrayIndexTwoBatches) {
166+
auto& vocbase = server->getSystemDatabase();
167+
auto fakeRequest = std::make_unique<GeneralRequestMock>(vocbase);
168+
auto fakeResponse = std::make_unique<GeneralResponseMock>();
169+
fakeRequest->setRequestType(arangodb::rest::RequestType::POST);
170+
fakeRequest->_payload.add(R"json(
171+
{
172+
"query": "FOR i IN 1..2000 RETURN CONCAT('', i)",
173+
"options": { "stream": true }
174+
}
175+
)json"_vpack);
176+
177+
auto* registry = arangodb::QueryRegistryFeature::registry();
178+
179+
auto testee =
180+
std::make_shared<arangodb::RestCursorHandler>(server->server(),
181+
fakeRequest.release(),
182+
fakeResponse.release(), registry);
183+
184+
testee->execute();
185+
186+
fakeResponse.reset(dynamic_cast<GeneralResponseMock*>(testee->stealResponse().release()));
187+
// this is necessary to reset the wakeup handler, which otherwise holds a
188+
// shared_ptr to testee.
189+
testee->shutdownExecute(true);
190+
191+
auto const responseBodySlice = fakeResponse->_payload.slice();
192+
193+
{ // release the query, so the AQL feature doesn't wait on it during shutdown
194+
auto const idSlice = responseBodySlice.get("id");
195+
ASSERT_FALSE(idSlice.isNone());
196+
ASSERT_TRUE(idSlice.isString());
197+
auto fakeRequest = std::make_unique<GeneralRequestMock>(vocbase);
198+
fakeRequest->setRequestType(arangodb::rest::RequestType::DELETE_REQ);
199+
fakeRequest->addSuffix(idSlice.copyString());
200+
auto fakeResponse = std::make_unique<GeneralResponseMock>();
201+
auto restHandler =
202+
std::make_shared<arangodb::RestCursorHandler>(server->server(),
203+
fakeRequest.release(),
204+
fakeResponse.release(), registry);
205+
restHandler->execute();
206+
fakeResponse.reset(
207+
dynamic_cast<GeneralResponseMock*>(testee->stealResponse().release()));
208+
}
209+
210+
testee.reset();
211+
212+
ASSERT_TRUE(responseBodySlice.isObject());
213+
214+
auto const resultSlice = responseBodySlice.get("result").resolveExternal();
215+
ASSERT_FALSE(resultSlice.isNone());
216+
ASSERT_TRUE(resultSlice.isArray())
217+
<< "Expected array, but got " << valueTypeName(resultSlice.type());
218+
// should be a compact array
219+
ASSERT_EQ(0x13, resultSlice.head());
220+
}

‎tests/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ set(ARANGODB_TESTS_SOURCES
130130
Aql/NoResultsExecutorTest.cpp
131131
Aql/ProjectionsTest.cpp
132132
Aql/QueryHelper.cpp
133+
Aql/QueryCursorTest.cpp
133134
Aql/RegisterPlanTest.cpp
134135
Aql/RemoteExecutorTest.cpp
135136
Aql/RemoveExecutorTest.cpp

‎tests/Mocks/Servers.cpp‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ MockMetricsServer::MockMetricsServer(bool start) : MockServer() {
322322
addFeature<arangodb::EngineSelectorFeature>(false);
323323

324324
if (start) {
325-
startFeatures();
325+
MockMetricsServer::startFeatures();
326326
}
327327
}
328328

@@ -332,7 +332,7 @@ MockV8Server::MockV8Server(bool start) : MockServer() {
332332
addFeature<arangodb::NetworkFeature>(false);
333333

334334
if (start) {
335-
startFeatures();
335+
MockV8Server::startFeatures();
336336
}
337337
}
338338

@@ -349,7 +349,7 @@ MockAqlServer::MockAqlServer(bool start) : MockServer() {
349349
SetupAqlPhase(*this);
350350

351351
if (start) {
352-
startFeatures();
352+
MockAqlServer::startFeatures();
353353
}
354354
}
355355

@@ -405,7 +405,7 @@ MockRestServer::MockRestServer(bool start) : MockServer() {
405405
addFeature<arangodb::QueryRegistryFeature>(false);
406406
addFeature<arangodb::NetworkFeature>(false);
407407
if (start) {
408-
startFeatures();
408+
MockRestServer::startFeatures();
409409
}
410410
}
411411

@@ -572,8 +572,8 @@ MockDBServer::MockDBServer(bool start) : MockClusterServer() {
572572
addFeature<arangodb::FlushFeature>(false); // do not start the thread
573573
addFeature<arangodb::MaintenanceFeature>(false); // do not start the thread
574574
if (start) {
575-
startFeatures();
576-
createDatabase("_system");
575+
MockDBServer::startFeatures();
576+
MockDBServer::createDatabase("_system");
577577
}
578578
}
579579

@@ -620,8 +620,8 @@ void MockDBServer::dropDatabase(std::string const& name) {
620620
MockCoordinator::MockCoordinator(bool start) : MockClusterServer() {
621621
arangodb::ServerState::instance()->setRole(arangodb::ServerState::RoleEnum::ROLE_COORDINATOR);
622622
if (start) {
623-
startFeatures();
624-
createDatabase("_system");
623+
MockCoordinator::startFeatures();
624+
MockCoordinator::createDatabase("_system");
625625
}
626626
}
627627

@@ -642,3 +642,9 @@ void MockCoordinator::dropDatabase(std::string const& name) {
642642
auto vocbase = databaseFeature.lookupDatabase(name);
643643
TRI_ASSERT(vocbase == nullptr);
644644
}
645+
646+
MockRestAqlServer::MockRestAqlServer() {
647+
SetupAqlPhase(*this);
648+
addFeature<arangodb::NetworkFeature>(false);
649+
MockRestAqlServer::startFeatures();
650+
}

‎tests/Mocks/Servers.h‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,8 @@ class MockServer {
110110
void stopFeatures();
111111

112112
protected:
113-
arangodb::application_features::ApplicationServer::State _oldApplicationServerState;
113+
arangodb::application_features::ApplicationServer::State _oldApplicationServerState =
114+
arangodb::application_features::ApplicationServer::State::UNINITIALIZED;
114115
arangodb::application_features::ApplicationServer _server;
115116
StorageEngineMock _engine;
116117
std::unordered_map<arangodb::application_features::ApplicationFeature*, bool> _features;
@@ -151,8 +152,8 @@ class MockAqlServer : public MockServer,
151152
public LogSuppressor<iresearch::TOPIC, LogLevel::FATAL>,
152153
public IResearchLogSuppressor {
153154
public:
154-
MockAqlServer(bool startFeatures = true);
155-
~MockAqlServer();
155+
explicit MockAqlServer(bool startFeatures = true);
156+
~MockAqlServer() override;
156157

157158
std::shared_ptr<arangodb::transaction::Methods> createFakeTransaction() const;
158159
// runBeforePrepare gives an entry point to modify the list of collections one want to use within the Query.
@@ -170,7 +171,17 @@ class MockRestServer : public MockServer,
170171
public LogSuppressor<iresearch::TOPIC, LogLevel::FATAL>,
171172
public IResearchLogSuppressor {
172173
public:
173-
MockRestServer(bool startFeatures = true);
174+
explicit MockRestServer(bool startFeatures = true);
175+
};
176+
177+
class MockRestAqlServer : public MockServer,
178+
public LogSuppressor<Logger::AUTHENTICATION, LogLevel::WARN>,
179+
public LogSuppressor<Logger::CLUSTER, LogLevel::ERR>,
180+
public LogSuppressor<Logger::FIXME, LogLevel::ERR>,
181+
public LogSuppressor<iresearch::TOPIC, LogLevel::FATAL>,
182+
public IResearchLogSuppressor {
183+
public:
184+
explicit MockRestAqlServer();
174185
};
175186

176187
class MockClusterServer : public MockServer,
@@ -205,7 +216,7 @@ class MockClusterServer : public MockServer,
205216
private:
206217
arangodb::ServerState::RoleEnum _oldRole;
207218
std::unique_ptr<AsyncAgencyStorePoolMock> _pool;
208-
int _dummy;
219+
int _dummy{};
209220
};
210221

211222
class MockDBServer : public MockClusterServer {

0 commit comments

Comments
 (0)
0