VectorDataReader

[Seddryck]

Hi @Giorgi

Yesterday when trying to work on the PR, I was a bit struggling with the concept of VectorDataReader so I spent a few additional minutes to review this class.

If my understanding is correct a vector in DuckDB is more or less a row and the class VectorDataReader allows us to read the value of a single field of this row. Is it correct (conceptually)?

[Giorgi]

No, VectorDataReader reads data for a specific column. The result object contains one or more chunks (depending on the number of rows in the result set) and each chunk has a vector for each column. If the column returns a composite type (List, Struct) the vector has child vector(s) that contain data. In the case of List, there is one child vector and in the case of Struct one vector for each property of struct.

[Giorgi]

You are probably using an older version of DuckDB that does integer division when using /

https://duckdb.org/docs/sql/functions/numeric.html

[Giorgi]

If you delete all bin/obj folders in the solution and rebuild the Test project, it will download the latest version of DuckDB.

[Seddryck]

Thx, it fixed the issue with the test.

Implemented for Struct and also added Blob which was missing in the factory. Initial implementation id done.
Feel free to review in the PR #153

[Giorgi]

What was missing for blobs?

[Seddryck]

Was my bad but didn't see it before because tests were on the namespace of parameters.

[Seddryck]

I've an issue with the consistency regarding the ClrTypes.

DuckDBType.Timestamp => typeof(DateTime),
DuckDBType.Interval => typeof(DuckDBInterval),
DuckDBType.Date => typeof(DuckDBDateOnly),
DuckDBType.Time => typeof(DuckDBTimeOnly),
DuckDBType.HugeInt => typeof(BigInteger),

I'm not sure why we are returning a BigInteger and not DuckDBHugeInt but DuckDBDateOnly and not DateOnly. I guess it's linked to .NET Standard 2.0 vs .NET 6.0 but IMO, it's a bit inconsistent.

[Giorgi]

Yeah, I agree. It's not consistent. I'll need some time to analyze what effect changing it will have.

[Seddryck]

Don't put too much effort in the analysis, we'll see that later. Could be easier to evaluate after refactoring. Keeping the inconsistency as-is at the moment.

[Seddryck]

DuckDBType.Enum => typeof(string), sounds also weird.

[Giorgi]

It's because GetValue returns a string for Enum columns.

[Seddryck]

A bit perplex about this case:

The original code in DuckDBDataReader was

public override bool GetBoolean(int ordinal)
{
    return GetByte(ordinal) != 0;
}

after refactoring, to pass the tests, I had to switch back to

public override bool GetBoolean(int ordinal)
{
    return GetFieldData<bool>(ordinal);
}

There was any reason to parse the boolean as a byte before? Am I missing something? (If yes we should change the tests because it's not capturing something).

[Seddryck]

Thinking out that I could read only the first bit of the byte and by luck getting the expected result. Will also check on my side.

[Giorgi]

There is no specific reason why I wrote it that way, it was the first thing that came to my mind. But I'm wondering if you are only refactoring and moving the code around, why would it cause that to fail. Either there was already a bug in GetBoolean that you uncovered now (which is completely possible) or you are doing something wrong.

[Seddryck]

I'm blocked somewhere with object GetEnum(ulong offset, Type returnType) which can return null at (line 239) return default!; Which means at the end that T GetFieldValue<T> in DuckDBDataReader can also return null which is not aligned with the SqlClient.SqlDataReader throwing an exception when it hits a null value. Also T GetFieldValue<T> is not supposed to return null or it would be T? but this method overrides a method defined in DbDataReader so we can't really changing this behavior.

That wouldn't be a big problem to change this behavior but there is a test specifically testing this behavior (SelectEnumsAsNullable).

What do we do? Back to the standard behavior or continue to cheat with the compiler?

[Seddryck]

I checked in Npgsql is also returning null in case of Nullable<>.

[Giorgi]

GetFieldValue will return null only for Nullable types and if the value is null. You are right about Postgres, I asked npgsql maintainer for an advice.