marcboeker / go-duckdb

go-duckdb provides a database/sql driver for the DuckDB database engine.
MIT License
623 stars 97 forks source link

Tables containing columns of type TIMESTAMPTZ cannot be queried. #172

Closed feiniks closed 5 months ago

feiniks commented 6 months ago

Hello, after updating to go-duckdb v1.6.1, if the table contains columns of type TIMESTAMPTZ, then the table cannot be queried properly and an error is returned: unsupported type 32. Can you confirm if this is a bug or if there are other types of columns that can be used in place of TIMESTAMPTZ. go-duckdb v1.5.6 is able to query TIMESTAMPTZ type columns properly.

marcboeker commented 6 months ago

Maybe this could be related? https://github.com/duckdb/duckdb/issues/9532

Do you have a test case to reproduce this?

feiniks commented 6 months ago

Maybe this could be related? duckdb/duckdb#9532

Do you have a test case to reproduce this?

Hi, here is a simple test case.

package main

import "fmt"
import "time"
import "database/sql"
import _ "github.com/marcboeker/go-duckdb"

func main() {
    db, err := sql.Open("duckdb", "olap.db")
    if err != nil {
        fmt.Println(err)
        return
    }
    defer db.Close()
    // install and load ICU extension
    _, err = db.Exec("INSTALL icu")
    if err != nil {
        fmt.Println(err)
        return
    }
    _, err = db.Exec("LOAD icu")
    if err != nil {
        fmt.Println(err)
        return
    }

    _, err = db.Exec("CREATE TABLE IF NOT EXISTS meta(uuid VARCHAR, tz TIMESTAMPTZ)")
    if err != nil {
        fmt.Println(err)
        return
    }
    _, err = db.Exec("INSERT INTO meta (uuid, tz) VALUES(?, ?)", "C0AC248F-C49A-4901-9AE0-61DBB415A1E9", time.Now())
    if err != nil {
        fmt.Println(err)
        return
    }
    sqlStr := "SELECT tz FROM meta"
    row := db.QueryRow(sqlStr)
    var tz time.Time
    if err := row.Scan(&tz); err != nil {
        fmt.Println("failed to scan:", err)
        return
    }
}
marcboeker commented 6 months ago

Thanks, I will have a look into it.

marcboeker commented 6 months ago

@taniabogatsch First look reveals that we're missing TIMESTAMP_TZ in the list of exposed types in the C API:

typedef enum DUCKDB_TYPE {
  DUCKDB_TYPE_INVALID,
  DUCKDB_TYPE_BOOLEAN,
  DUCKDB_TYPE_TINYINT,
  DUCKDB_TYPE_SMALLINT,
  DUCKDB_TYPE_INTEGER,
  DUCKDB_TYPE_BIGINT,
  DUCKDB_TYPE_UTINYINT,
  DUCKDB_TYPE_USMALLINT,
  DUCKDB_TYPE_UINTEGER,
  DUCKDB_TYPE_UBIGINT,
  DUCKDB_TYPE_FLOAT,
  DUCKDB_TYPE_DOUBLE,
  DUCKDB_TYPE_TIMESTAMP,
  DUCKDB_TYPE_DATE,
  DUCKDB_TYPE_TIME,
  DUCKDB_TYPE_INTERVAL,
  DUCKDB_TYPE_HUGEINT,
  DUCKDB_TYPE_VARCHAR,
  DUCKDB_TYPE_BLOB,
  DUCKDB_TYPE_DECIMAL,
  DUCKDB_TYPE_TIMESTAMP_S,
  DUCKDB_TYPE_TIMESTAMP_MS,
  DUCKDB_TYPE_TIMESTAMP_NS,
  DUCKDB_TYPE_ENUM,
  DUCKDB_TYPE_LIST,
  DUCKDB_TYPE_STRUCT,
  DUCKDB_TYPE_MAP,
  DUCKDB_TYPE_UUID,
  DUCKDB_TYPE_UNION,
  DUCKDB_TYPE_BIT,
} duckdb_type;

Do you know if it is planned to support TIMESTAMP_TZ in the C API?

Mapping TIMESTAMP_TZ to TIMESTAMP works, but it's probably missing the TZ information. But the types seem to be compatible in some way:

Adding to rows.go line 132:

case 32:
    return time.UnixMicro(int64(get[C.duckdb_timestamp](vector, rowIdx).micros)).UTC(), nil
taniabogatsch commented 6 months ago

Potentially fixed by https://github.com/duckdb/duckdb/pull/9539. Did you try with the latest duckdb main branch instead of v0.10.0? Otherwise, having another look once duckdb goes from v0.10.0 to v0.10.1 makes sense.

I'll also see what can be done/is necessary to solve this from the C API side.

hawkfish commented 6 months ago

@marcboeker In v0.10.0, the enum now has fixed values and has the missing types at the end:

    // duckdb_bit
    DUCKDB_TYPE_BIT = 29,
    // duckdb_time_tz
    DUCKDB_TYPE_TIME_TZ = 30,
    // duckdb_timestamp
    DUCKDB_TYPE_TIMESTAMP_TZ = 31,
} duckdb_type;

Ideally TS and TSTZ contain instants, so they should be interchangeable, but if Go's timestamp type optionally supports time zones (for binning operations) then ideally you should read the TimeZone setting from the ClientContext and use that instead of UTC.

The release tag is just v0.10.0 so git checkout -b v0.10.0 v0.10.0 should get you started.

michaelmdresser commented 6 months ago

the enum now has fixed values and has the missing types at the end

I thought that was fixed in a patch after v0.10.0, based on my reading of https://github.com/duckdb/duckdb/issues/10634#issuecomment-1942323326, meaning go-duckdb will have to wait for v0.10.1 of DuckDB.

hawkfish commented 6 months ago

Ah my bad - I thought I'd switched to the branch but apparently I hadn't. The enum identifiers are there in v0.10.0 but they haven't had their values nailed down:

    DUCKDB_TYPE_BIT,
    // duckdb_time_tz
    DUCKDB_TYPE_TIME_TZ,
    // duckdb_timestamp
    DUCKDB_TYPE_TIMESTAMP_TZ,
} duckdb_type;

So you should be able to use the symbolic constants with the latest API release without worrying about the values changing.

hawkfish commented 6 months ago

I got tripped up by naming the branch and the tag the same, so git checkout -b release-0-10-0 v0.10.0...

niger-prequel commented 6 months ago

👋 @marcboeker @taniabogatsch we tried to run this with DuckDB compiled against 8b3b3cffccccd8523236ab2e72544ec114993526 which is presently head. Instead of unsupported type 32, we now get unsupported type 31.

michaelmdresser commented 6 months ago

Did you update the header file too? make deps.header. I'm not sure if that would have an impact, but its what I would suspect.

marcboeker commented 5 months ago

@niger-prequel Have you got it working with what @michaelmdresser has suggested?

niger-prequel commented 5 months ago

@marcboeker we just tried that today and it didn't work, same error (unsupported type 31).

taniabogatsch commented 5 months ago

I've opened a PR to address this: https://github.com/marcboeker/go-duckdb/pull/180. I would appreciate feedback if you have any. It might even be worth pulling the PR and seeing if it fixes your issues; cc @niger-prequel @feiniks

feiniks commented 5 months ago

Hello @taniabogatsch, this PR works for me.