Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Expose LogicalType(s) for columns in QueryResult, fix #24 #25

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+52 −13
Open

feat: Expose LogicalType(s) for columns in QueryResult, fix #24 #25

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f0e40e7
feat: Expose LogicalType(s) for columns in QueryResult, fix #24
ovr Nov 2, 2023
693e2a4
chore: type
ovr Nov 2, 2023
6169c7b
chore: correct type
ovr Nov 2, 2023
dd42ca9
chore: review, same api as Statement.columns()
ovr Nov 2, 2023
39f4c4f
chore: column can return null only for Statement
ovr Nov 2, 2023
7a66f67
chore: assert
ovr Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lib/duckdb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,15 @@ export class Connection {
): void;
unregister_udf(name: string, callback: Callback<any>): void;

stream(sql: any, ...args: any[]): QueryResult;
stream(sql: any, ...args: any[]): Promise<QueryResult>;
arrowIPCStream(sql: any, ...args: any[]): Promise<IpcResultStreamIterator>;

register_buffer(name: string, array: ArrowIterable, force: boolean, callback?: Callback<void>): void;
unregister_buffer(name: string, callback?: Callback<void>): void;
}

export class QueryResult implements AsyncIterable<RowData> {
columns(): ColumnInfo[] | null;
[Symbol.asyncIterator](): AsyncIterator<RowData>;
}

Expand Down Expand Up @@ -166,7 +167,7 @@ export class Database {
): void;
unregister_udf(name: string, callback: Callback<any>): void;

stream(sql: any, ...args: any[]): QueryResult;
stream(sql: any, ...args: any[]): Promise<QueryResult>;
arrowIPCStream(sql: any, ...args: any[]): Promise<IpcResultStreamIterator>;

serialize(done?: Callback<void>): void;
Expand Down
17 changes: 11 additions & 6 deletions lib/duckdb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ QueryResult.prototype.nextChunk;
*/
QueryResult.prototype.nextIpcBuffer;

/**
* Function to return logical types for columns
*
* @method
* @return {ColumnInfo[]} - Array of column names and types
*/
QueryResult.prototype.columns;

/**
* @name asyncIterator
* @memberof module:duckdb~QueryResult
Expand Down Expand Up @@ -218,12 +226,9 @@ Connection.prototype.each = function (sql) {
* @param {...*} params
* @yields row chunks
*/
Connection.prototype.stream = async function* (sql) {
Connection.prototype.stream = async function (sql) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description of change:

The old version used AsyncIterator (function was marked with *), which will lead to the wrong return type (not QueryResult) + unexpected iteration over another iterator.

Thanks

Copy link
Collaborator

@carlopi carlopi Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What problem does this solve?
This will break the current API and require changes elsewhere that is doable but unsure if it makes sense here.
Could the GetColumns itself not be an async function?

Copy link
Author

@ovr ovr Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It started to return QueryResult which is declared in typings, when the old one return AsyncGenerator.
It's not possible to call getColumns on top of AsyncIterator.

Example to demo that async * returns AsyncGenerator.

❯ node
Welcome to Node.js v16.19.1.
Type ".help" for more information.
> class MyClass { kek() { return true }; };
undefined
> const fn = async function* () { return new MyClass(); }
undefined
> console.log(fn())
Object [AsyncGenerator] {}
undefined

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current version returns a class wrapping an AsyncGenerator.

Something like:

class MyClass {
        someSyncMethod() {                                                                                 
                return 0;
                }
        async someAsyncMethod() {
                await setTimeout(1);
                return 42;
                }
        async *[Symbol.asyncIterator]() { 
                for (var i = 0; i<10; i++)
                        yield i;
        }
};

An instance of this class can behave like an async generator (so var c = new MyClass; for await (var obj of c) ..., but can also have methods of its own like c.someSyncMethod() or await c.someAsyncMethod().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The biggest problem that, old variant is an AsyncGenerator on top of QueryResult (local scoped), which implements asyncIterator.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I see the problem as you are seeing it, could you go over it once again?

Copy link
Author

@ovr ovr Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

image

is it clear?

const statement = new Statement(this, sql);
const queryResult = await statement.stream.apply(statement, arguments);
for await (const result of queryResult) {
yield result;
}
return statement.stream.apply(statement, arguments);
}

/**
Expand Down Expand Up @@ -713,7 +718,7 @@ Statement.prototype.sql;

/**
* @method
* @return {ColumnInfo[]} - Array of column names and types
* @return {ColumnInfo[] | null} - Array of column names and types
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prof:

image

*/
Statement.prototype.columns;

Expand Down
1 change: 1 addition & 0 deletions src/duckdb_node.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ class QueryResult : public Napi::ObjectWrap<QueryResult> {
public:
Napi::Value NextChunk(const Napi::CallbackInfo &info);
Napi::Value NextIpcBuffer(const Napi::CallbackInfo &info);
Napi::Value Columns(const Napi::CallbackInfo &info);
duckdb::shared_ptr<ArrowSchema> cschema;

private:
Expand Down
23 changes: 22 additions & 1 deletion src/statement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,8 @@ Napi::FunctionReference QueryResult::Init(Napi::Env env, Napi::Object exports) {

Napi::Function t = DefineClass(env, "QueryResult",
{InstanceMethod("nextChunk", &QueryResult::NextChunk),
InstanceMethod("nextIpcBuffer", &QueryResult::NextIpcBuffer)});
InstanceMethod("nextIpcBuffer", &QueryResult::NextIpcBuffer),
InstanceMethod("columns", &QueryResult::Columns)});

exports.Set("QueryResult", t);

Expand Down Expand Up @@ -742,6 +743,26 @@ Napi::Value QueryResult::NextIpcBuffer(const Napi::CallbackInfo &info) {
return deferred.Promise();
}

Napi::Value QueryResult::Columns(const Napi::CallbackInfo &info)
{
auto env = info.Env();
auto result = Napi::Array::New(env, this->result->ColumnCount());

for (duckdb::idx_t column_idx = 0; column_idx < this->result->ColumnCount(); column_idx++)
{
auto column_name = this->result->ColumnName(column_idx);
auto column_type = this->result->types[column_idx];

auto obj = Napi::Object::New(env);
obj.Set("name", Napi::String::New(env, column_name));
obj.Set("type", TypeToObject(env, column_type));

result.Set(column_idx, obj);
}

return result;
}

Napi::Object QueryResult::NewInstance(const Napi::Object &db) {
return NodeDuckDB::GetData(db.Env())->query_result_constructor.New({db});
}
Expand Down
13 changes: 12 additions & 1 deletion test/query_result.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,18 @@ describe('QueryResult', () => {

it('streams results', async () => {
let retrieved = 0;
const stream = conn.stream('SELECT * FROM range(0, ?)', total);

const stream = await conn.stream("SELECT * FROM range(0, ?)", total);
assert.deepEqual(stream.columns(), [
{
name: 'range',
type: {
id: 'BIGINT',
sql_type: 'BIGINT',
}
}
]);

for await (const row of stream) {
retrieved++;
}
Expand Down
2 changes: 1 addition & 1 deletion test/typescript_decls.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ describe("typescript: stream and QueryResult", function () {

it("streams results", async () => {
let retrieved = 0;
const stream = conn.stream("SELECT * FROM range(0, ?)", total);
const stream = await conn.stream("SELECT * FROM range(0, ?)", total);
for await (const row of stream) {
retrieved++;
}
Expand Down