nakagami / firebirdsql

Firebird RDBMS sql driver for Go (golang)
MIT License
220 stars 60 forks source link

xsqlvar.go: fix varchar (VARYING) type display length #152

Closed the-goodies closed 1 year ago

the-goodies commented 1 year ago

Length method of rows.ColumnTypes returns incorrect length (always -1) for VARCHAR (VARYING) type, while returning correct length for CHAR (TEXT) type. This patch fixes this. Applying change only to displayLength method seems to be sufficient, but I changed ioLength too (not sure if necessary).

nakagami commented 1 year ago

To me, the code before the modification seems correct. In the case of varchar, the length is taken from the fetched data, not from the definition.

If this modification is correct, please rewrite it to add tests and make sure everything passes.

the-goodies commented 1 year ago

In the case of varchar, the length is taken from the fetched data, not from the definition.

Here's code example how I take length:

    rows, err := db.db.Queryx(fmt.Sprintf("SELECT * FROM %s ROWS 0", structTypeName(v)))
    if err != nil {
        return nil, fault.Wrap(err)
    }
    defer rows.Close()

    colTypes, err := rows.ColumnTypes()
    if err != nil {
        return nil, fault.Wrap(err)
    }
    for _, t := range colTypes {
        length, _ := t.Length()
        fmt.Println(t.Name(), length)
    }

I fetch 0 ROWS, and even if I'm fetching all rows, it correctly returns varchar defined length (not actual rows columns data length).

If this modification is correct, please rewrite it to add tests and make sure everything passes.

Sure, I could, but already existing test's timeout after 10 min. on my computer and as we can see on GitHub Actions too. What is the problem with that?

nakagami commented 1 year ago

I think I need to make another modification because the test seems to fail with this pull request. (I guess the VarChar data cannot be fetched?)

However, I understand what you mean by Length() not being able to get the length of an example.

I'll think about it when I have time.

the-goodies commented 1 year ago

but I changed ioLength too (not sure if necessary).

It seems this was the culprit for tests not running (timing out), but not sure why. Anyway, for my use case, ioLength() wasn't necessary anyway.

nakagami commented 1 year ago

I see, that sounds good, as long as only displayLength() is modified.