Feature: improve duckdbDataSource performance #278
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
onlyjackfrost
force-pushed
the
chore/performance-analysis
branch
from
827e771 to
c52cec8
Compare
…queries, and use connection.all to speed up
onlyjackfrost
force-pushed
the
chore/performance-analysis
branch
from
f3f1048 to
bd598ab
Compare
onlyjackfrost
force-pushed
the
chore/performance-analysis
branch
from
2590a27 to
0936c5c
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #278 +/- ##
===========================================
- Coverage 90.21% 90.08% -0.13%
===========================================
Files 350 351 +1
Lines 5967 6044 +77
Branches 803 816 +13
===========================================
+ Hits 5383 5445 +62
- Misses 423 437 +14
- Partials 161 162 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
onlyjackfrost
force-pushed
the
chore/performance-analysis
branch
from
659e429 to
fdcfc27
Compare
onlyjackfrost
changed the title
| // write to txt file | ||
| public static writePerformanceReport() { | ||
| const filePath = path.join('./performanceRecord.txt'); | ||
| // print current date, time as humun readable format |
| @@ -99,49 +99,74 @@ export class DuckDBDataSource extends DataSource<any, DuckDBOptions> { | |||
| } | |||
| const { db, configurationParameters, ...options } = | |||
| this.dbMapping.get(profileName)!; | |||
| const builtSQL = buildSQL(sql, operations); | |||
| const [builtSQL, streamSQL] = buildSQL(sql, operations); | |||
There was a problem hiding this comment.
Suggest commenting on the purpose of why do you separate two SQL
|
|
||
| const result = await statement.stream(...parameters); | ||
| const firstChunk = await result.nextChunk(); | ||
| const [result, asyncIterable] = await Promise.all([ |
There was a problem hiding this comment.
Suggest you comment on why you separate two SQL and call one by .all and call another by stream, do let members know the reason why not call .all and convert to stream only or use .stream only
|
|
||
| const result = await statement.stream(...parameters); | ||
| const firstChunk = await result.nextChunk(); | ||
| const [result, asyncIterable] = await Promise.all([ |
There was a problem hiding this comment.
Could you use an other name to replace asyncIterable ? not straightforward actually.
| } | ||
| }), | ||
| ]); | ||
| const asyncIterableStream = new Readable({ |
There was a problem hiding this comment.
Could you create a method to refactor the execution to prevent too long to read, e.g: convertToStream(result, iterable)
| // set duckdb thread to number | ||
| private async setThread(db: duckdb.Database) { | ||
| const thread = process.env['THREADS']; | ||
|
|
||
| if (!thread) return; | ||
| await new Promise((resolve, reject) => { | ||
| db.run(`SET threads=${Number(thread)}`, (err: any) => { | ||
| if (err) reject(err); | ||
| this.logger.debug(`Set thread to ${thread}`); | ||
| resolve(true); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
After discussion, if you still would like to keep the method, please comment the method's purpose and add the usage way in the README
| @@ -26,15 +26,23 @@ export const isNoOP = ( | |||
| return true; | |||
| }; | |||
|
|
|||
| const duckdbExecuteChunkSize = | |||
| process.env['DUCKDB_EXECUTE_CHUNK_SIZE'] || '2000'; | |||
There was a problem hiding this comment.
Suggest to update to mentioned the env in README, and what influence will be if set the env to large value or small value.
onlyjackfrost
force-pushed
the
chore/performance-analysis
branch
from
b4980f9 to
75ab334
Compare
…t variable description in READ.me
onlyjackfrost
force-pushed
the
chore/performance-analysis
branch
from
75ab334 to
58db508
Compare
Description
Note:
With the same data, the performance of stream.nextChunk is 1.5~2 times of the performance of connection.all