Skip to content

Commit 5030d0f

Browse files
kravets-levkokravets-levko
authored
[PECO-1258] Don't fetch results beyond the end of dataset (#205)
* Refine conditions of results availability and adjust tests Signed-off-by: Levko Kravets <levko.ne@gmail.com> * [PECO-1258] Don't fetch results beyond the end of dataset Signed-off-by: Levko Kravets <levko.ne@gmail.com> * Remove irrelevant changes Signed-off-by: Levko Kravets <levko.ne@gmail.com> --------- Signed-off-by: Levko Kravets <levko.ne@gmail.com>
1 parent bdc7126 commit 5030d0f

4 files changed

Lines changed: 69 additions & 39 deletions

File tree

‎lib/DBSQLOperation/index.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,8 +68,6 @@ export default class DBSQLOperation implements IOperation {
6868
// to `getOperationStatus()` may fail with irrelevant errors, e.g. HTTP 404
6969
private operationStatus?: TGetOperationStatusResp;
7070

71-
private hasResultSet: boolean = false;
72-
7371
private resultHandler?: ResultSlicer<any>;
7472

7573
constructor({ handle, directResults, context }: DBSQLOperationConstructorOptions) {
@@ -78,7 +76,6 @@ export default class DBSQLOperation implements IOperation {
7876

7977
const useOnlyPrefetchedResults = Boolean(directResults?.closeOperation);
8078

81-
this.hasResultSet = this.operationHandle.hasResultSet;
8279
if (directResults?.operationStatus) {
8380
this.processOperationStatusResponse(directResults.operationStatus);
8481
}
@@ -139,7 +136,7 @@ export default class DBSQLOperation implements IOperation {
139136
public async fetchChunk(options?: FetchOptions): Promise<Array<object>> {
140137
await this.failIfClosed();
141138

142-
if (!this.hasResultSet) {
139+
if (!this.operationHandle.hasResultSet) {
143140
return [];
144141
}
145142

@@ -271,7 +268,7 @@ export default class DBSQLOperation implements IOperation {
271268
public async getSchema(options?: GetSchemaOptions): Promise<TTableSchema | null> {
272269
await this.failIfClosed();
273270

274-
if (!this.hasResultSet) {
271+
if (!this.operationHandle.hasResultSet) {
275272
return null;
276273
}
277274

@@ -412,7 +409,7 @@ export default class DBSQLOperation implements IOperation {
412409
this.state = response.operationState ?? this.state;
413410

414411
if (typeof response.hasResultSet === 'boolean') {
415-
this.hasResultSet = response.hasResultSet;
412+
this.operationHandle.hasResultSet = response.hasResultSet;
416413
}
417414

418415
const isInProgress = [

‎lib/result/RowSetProvider.ts‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,16 @@ export default class RowSetProvider implements IResultsProvider<TRowSet | undefi
4747

4848
private readonly returnOnlyPrefetchedResults: boolean;
4949

50-
public hasMoreRows: boolean = false;
50+
private hasMoreRowsFlag?: boolean = undefined;
51+
52+
private get hasMoreRows(): boolean {
53+
// `hasMoreRowsFlag` is populated only after fetching the first row set.
54+
// Prior to that, we use a `operationHandle.hasResultSet` flag which
55+
// is set if there are any data at all. Also, we have to choose appropriate
56+
// flag in a getter because both `hasMoreRowsFlag` and `operationHandle.hasResultSet`
57+
// may change between this getter calls
58+
return this.hasMoreRowsFlag ?? this.operationHandle.hasResultSet;
59+
}
5160

5261
constructor(
5362
context: IClientContext,
@@ -68,15 +77,7 @@ export default class RowSetProvider implements IResultsProvider<TRowSet | undefi
6877
private processFetchResponse(response: TFetchResultsResp): TRowSet | undefined {
6978
Status.assert(response.status);
7079
this.fetchOrientation = TFetchOrientation.FETCH_NEXT;
71-
72-
if (this.prefetchedResults.length > 0) {
73-
this.hasMoreRows = true;
74-
} else if (this.returnOnlyPrefetchedResults) {
75-
this.hasMoreRows = false;
76-
} else {
77-
this.hasMoreRows = checkIfOperationHasMoreRows(response);
78-
}
79-
80+
this.hasMoreRowsFlag = checkIfOperationHasMoreRows(response);
8081
return response.results;
8182
}
8283

@@ -86,6 +87,16 @@ export default class RowSetProvider implements IResultsProvider<TRowSet | undefi
8687
return this.processFetchResponse(prefetchedResponse);
8788
}
8889

90+
// We end up here if no more prefetched results available (checked above)
91+
if (this.returnOnlyPrefetchedResults) {
92+
return undefined;
93+
}
94+
95+
// Don't fetch next chunk if there are no more data available
96+
if (!this.hasMoreRows) {
97+
return undefined;
98+
}
99+
89100
const driver = await this.context.getDriver();
90101
const response = await driver.fetchResults({
91102
operationHandle: this.operationHandle,
@@ -98,6 +109,16 @@ export default class RowSetProvider implements IResultsProvider<TRowSet | undefi
98109
}
99110

100111
public async hasMore() {
112+
// If there are prefetched results available - return `true` regardless of
113+
// the actual state of `hasMoreRows` flag (because we actually have some data)
114+
if (this.prefetchedResults.length > 0) {
115+
return true;
116+
}
117+
// We end up here if no more prefetched results available (checked above)
118+
if (this.returnOnlyPrefetchedResults) {
119+
return false;
120+
}
121+
101122
return this.hasMoreRows;
102123
}
103124
}

‎tests/e2e/cloudfetch.test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ describe('CloudFetch', () => {
4242
LEFT JOIN (SELECT 1) AS t2
4343
`,
4444
{
45-
maxRows: 100000,
45+
maxRows: null, // disable DirectResults
4646
useCloudFetch: true, // tell server that we would like to use CloudFetch
4747
},
4848
);
@@ -62,7 +62,7 @@ describe('CloudFetch', () => {
6262
// With the count of rows we queried, there should be at least one row set,
6363
// containing 8 result links. After fetching the first chunk,
6464
// result handler should download 5 of them and schedule the rest
65-
expect(await cfResultHandler.hasMore()).to.be.false;
65+
expect(await cfResultHandler.hasMore()).to.be.true;
6666
expect(cfResultHandler.pendingLinks.length).to.be.equal(0);
6767
expect(cfResultHandler.downloadTasks.length).to.be.equal(0);
6868

‎tests/unit/DBSQLOperation.test.js‎

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ describe('DBSQLOperation', () => {
129129
const operation = new DBSQLOperation({ handle, context });
130130

131131
expect(operation.state).to.equal(TOperationState.INITIALIZED_STATE);
132-
expect(operation.hasResultSet).to.be.true;
132+
expect(operation.operationHandle.hasResultSet).to.be.true;
133133
});
134134

135135
it('should pick up state from directResults', async () => {
@@ -149,7 +149,7 @@ describe('DBSQLOperation', () => {
149149
});
150150

151151
expect(operation.state).to.equal(TOperationState.FINISHED_STATE);
152-
expect(operation.hasResultSet).to.be.true;
152+
expect(operation.operationHandle.hasResultSet).to.be.true;
153153
});
154154

155155
it('should fetch status and update internal state', async () => {
@@ -165,14 +165,14 @@ describe('DBSQLOperation', () => {
165165
const operation = new DBSQLOperation({ handle, context });
166166

167167
expect(operation.state).to.equal(TOperationState.INITIALIZED_STATE);
168-
expect(operation.hasResultSet).to.be.false;
168+
expect(operation.operationHandle.hasResultSet).to.be.false;
169169

170170
const status = await operation.status();
171171

172172
expect(context.driver.getOperationStatus.called).to.be.true;
173173
expect(status.operationState).to.equal(TOperationState.FINISHED_STATE);
174174
expect(operation.state).to.equal(TOperationState.FINISHED_STATE);
175-
expect(operation.hasResultSet).to.be.true;
175+
expect(operation.operationHandle.hasResultSet).to.be.true;
176176
});
177177

178178
it('should request progress', async () => {
@@ -204,7 +204,7 @@ describe('DBSQLOperation', () => {
204204
const operation = new DBSQLOperation({ handle, context });
205205

206206
expect(operation.state).to.equal(TOperationState.INITIALIZED_STATE);
207-
expect(operation.hasResultSet).to.be.false;
207+
expect(operation.operationHandle.hasResultSet).to.be.false;
208208

209209
// First call - should fetch data and cache
210210
context.driver.getOperationStatusResp = {
@@ -216,7 +216,7 @@ describe('DBSQLOperation', () => {
216216
expect(context.driver.getOperationStatus.callCount).to.equal(1);
217217
expect(status1.operationState).to.equal(TOperationState.FINISHED_STATE);
218218
expect(operation.state).to.equal(TOperationState.FINISHED_STATE);
219-
expect(operation.hasResultSet).to.be.true;
219+
expect(operation.operationHandle.hasResultSet).to.be.true;
220220

221221
// Second call - should return cached data
222222
context.driver.getOperationStatusResp = {
@@ -228,7 +228,7 @@ describe('DBSQLOperation', () => {
228228
expect(context.driver.getOperationStatus.callCount).to.equal(1);
229229
expect(status2.operationState).to.equal(TOperationState.FINISHED_STATE);
230230
expect(operation.state).to.equal(TOperationState.FINISHED_STATE);
231-
expect(operation.hasResultSet).to.be.true;
231+
expect(operation.operationHandle.hasResultSet).to.be.true;
232232
});
233233

234234
it('should fetch status if directResults status is not finished', async () => {
@@ -254,14 +254,14 @@ describe('DBSQLOperation', () => {
254254
});
255255

256256
expect(operation.state).to.equal(TOperationState.RUNNING_STATE); // from directResults
257-
expect(operation.hasResultSet).to.be.false;
257+
expect(operation.operationHandle.hasResultSet).to.be.false;
258258

259259
const status = await operation.status(false);
260260

261261
expect(context.driver.getOperationStatus.called).to.be.true;
262262
expect(status.operationState).to.equal(TOperationState.FINISHED_STATE);
263263
expect(operation.state).to.equal(TOperationState.FINISHED_STATE);
264-
expect(operation.hasResultSet).to.be.true;
264+
expect(operation.operationHandle.hasResultSet).to.be.true;
265265
});
266266

267267
it('should not fetch status if directResults status is finished', async () => {
@@ -287,14 +287,14 @@ describe('DBSQLOperation', () => {
287287
});
288288

289289
expect(operation.state).to.equal(TOperationState.FINISHED_STATE); // from directResults
290-
expect(operation.hasResultSet).to.be.false;
290+
expect(operation.operationHandle.hasResultSet).to.be.false;
291291

292292
const status = await operation.status(false);
293293

294294
expect(context.driver.getOperationStatus.called).to.be.false;
295295
expect(status.operationState).to.equal(TOperationState.FINISHED_STATE);
296296
expect(operation.state).to.equal(TOperationState.FINISHED_STATE);
297-
expect(operation.hasResultSet).to.be.false;
297+
expect(operation.operationHandle.hasResultSet).to.be.false;
298298
});
299299

300300
it('should throw an error in case of a status error', async () => {
@@ -1025,6 +1025,7 @@ describe('DBSQLOperation', () => {
10251025
handle.hasResultSet = true;
10261026

10271027
context.driver.getOperationStatusResp.operationState = TOperationState.FINISHED_STATE;
1028+
context.driver.getOperationStatusResp.hasResultSet = true;
10281029
sinon.spy(context.driver, 'getResultSetMetadata');
10291030
sinon.spy(context.driver, 'fetchResults');
10301031

@@ -1175,20 +1176,23 @@ describe('DBSQLOperation', () => {
11751176
});
11761177

11771178
describe('hasMoreRows', () => {
1178-
it('should return False until first chunk of data fetched', async () => {
1179+
it('should return initial value prior to first fetch', async () => {
11791180
const context = new ClientContextMock();
11801181

11811182
const handle = new OperationHandleMock();
11821183
handle.hasResultSet = true;
11831184

11841185
context.driver.getOperationStatusResp.operationState = TOperationState.FINISHED_STATE;
11851186
context.driver.getOperationStatusResp.hasResultSet = true;
1186-
context.driver.fetchResultsResp.hasMoreRows = true;
1187+
context.driver.fetchResultsResp.hasMoreRows = false;
1188+
context.driver.fetchResultsResp.results = undefined;
11871189
const operation = new DBSQLOperation({ handle, context });
11881190

1189-
expect(await operation.hasMoreRows()).to.be.false;
1190-
await operation.fetchChunk({ disableBuffering: true });
11911191
expect(await operation.hasMoreRows()).to.be.true;
1192+
expect(operation._data.hasMoreRowsFlag).to.be.undefined;
1193+
await operation.fetchChunk({ disableBuffering: true });
1194+
expect(await operation.hasMoreRows()).to.be.false;
1195+
expect(operation._data.hasMoreRowsFlag).to.be.false;
11921196
});
11931197

11941198
it('should return False if operation was closed', async () => {
@@ -1202,7 +1206,7 @@ describe('DBSQLOperation', () => {
12021206
context.driver.fetchResultsResp.hasMoreRows = true;
12031207
const operation = new DBSQLOperation({ handle, context });
12041208

1205-
expect(await operation.hasMoreRows()).to.be.false;
1209+
expect(await operation.hasMoreRows()).to.be.true;
12061210
await operation.fetchChunk({ disableBuffering: true });
12071211
expect(await operation.hasMoreRows()).to.be.true;
12081212
await operation.close();
@@ -1220,7 +1224,7 @@ describe('DBSQLOperation', () => {
12201224
context.driver.fetchResultsResp.hasMoreRows = true;
12211225
const operation = new DBSQLOperation({ handle, context });
12221226

1223-
expect(await operation.hasMoreRows()).to.be.false;
1227+
expect(await operation.hasMoreRows()).to.be.true;
12241228
await operation.fetchChunk({ disableBuffering: true });
12251229
expect(await operation.hasMoreRows()).to.be.true;
12261230
await operation.cancel();
@@ -1238,9 +1242,11 @@ describe('DBSQLOperation', () => {
12381242
context.driver.fetchResultsResp.hasMoreRows = true;
12391243
const operation = new DBSQLOperation({ handle, context });
12401244

1241-
expect(await operation.hasMoreRows()).to.be.false;
1245+
expect(await operation.hasMoreRows()).to.be.true;
1246+
expect(operation._data.hasMoreRowsFlag).to.be.undefined;
12421247
await operation.fetchChunk({ disableBuffering: true });
12431248
expect(await operation.hasMoreRows()).to.be.true;
1249+
expect(operation._data.hasMoreRowsFlag).to.be.true;
12441250
});
12451251

12461252
it('should return True if hasMoreRows flag is False but there is actual data', async () => {
@@ -1254,9 +1260,11 @@ describe('DBSQLOperation', () => {
12541260
context.driver.fetchResultsResp.hasMoreRows = false;
12551261
const operation = new DBSQLOperation({ handle, context });
12561262

1257-
expect(await operation.hasMoreRows()).to.be.false;
1263+
expect(await operation.hasMoreRows()).to.be.true;
1264+
expect(operation._data.hasMoreRowsFlag).to.be.undefined;
12581265
await operation.fetchChunk({ disableBuffering: true });
12591266
expect(await operation.hasMoreRows()).to.be.true;
1267+
expect(operation._data.hasMoreRowsFlag).to.be.true;
12601268
});
12611269

12621270
it('should return True if hasMoreRows flag is unset but there is actual data', async () => {
@@ -1270,9 +1278,11 @@ describe('DBSQLOperation', () => {
12701278
context.driver.fetchResultsResp.hasMoreRows = undefined;
12711279
const operation = new DBSQLOperation({ handle, context });
12721280

1273-
expect(await operation.hasMoreRows()).to.be.false;
1281+
expect(await operation.hasMoreRows()).to.be.true;
1282+
expect(operation._data.hasMoreRowsFlag).to.be.undefined;
12741283
await operation.fetchChunk({ disableBuffering: true });
12751284
expect(await operation.hasMoreRows()).to.be.true;
1285+
expect(operation._data.hasMoreRowsFlag).to.be.true;
12761286
});
12771287

12781288
it('should return False if hasMoreRows flag is False and there is no data', async () => {
@@ -1287,9 +1297,11 @@ describe('DBSQLOperation', () => {
12871297
context.driver.fetchResultsResp.results = undefined;
12881298
const operation = new DBSQLOperation({ handle, context });
12891299

1290-
expect(await operation.hasMoreRows()).to.be.false;
1300+
expect(await operation.hasMoreRows()).to.be.true;
1301+
expect(operation._data.hasMoreRowsFlag).to.be.undefined;
12911302
await operation.fetchChunk({ disableBuffering: true });
12921303
expect(await operation.hasMoreRows()).to.be.false;
1304+
expect(operation._data.hasMoreRowsFlag).to.be.false;
12931305
});
12941306
});
12951307
});

0 commit comments

Comments
 (0)