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

[ovr]

Hello!

Fix - #24

Thanks

[ovr]

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

[carlopi]

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?

[ovr]

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

[carlopi]

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().

[ovr]

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

[carlopi]

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

[ovr]

is it clear?

[Maxxen]

Perhaps it would be better do something similar to the Columns method on statements instead of just returning the type id and name:

duckdb-node/src/statement.cpp

Line 588 in 2722553

Napi::Value Statement::Columns(const Napi::CallbackInfo &info) {

if you were to use the TypeToObject function (feel free to move it) youd get the extra type info recursively so that you can determine e.g. the width of decimals or the member types of structs.

[ovr]

Prof:

[ovr]