marcboeker / go-duckdb

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

Add MarshalText method for Decimal #223

Closed k-anshul closed 4 months ago

k-anshul commented 4 months ago

Adds MarshalText method for Decimal struct similar to big.Float of standard library

taniabogatsch commented 4 months ago

Hi @k-anshul, thanks for the PR. The changes look good. But I am unsure about is whether this is a function that has to live inside go-duckdb. I don't see a similar function for other types, i.e., it seems very specific. Could this be something that developers embed on their side of the application?

type Decimal struct {
    *duckdb.Decimal
}

func (d Decimal) MarshalText() {
    // ...
}
k-anshul commented 4 months ago

It is more of a convenient way to directly serialize results obtained from the library to JSON/string. I can add it at application level as well but thought it will be useful to add it directly to the original type since similar function also exists for big.Float?

k-anshul commented 4 months ago

Many drivers do not expose custom types but as an example here is a MarshalText method for UUID in mssql driver which has a custom type for handling UUIDs : https://github.com/denisenkom/go-mssqldb/blob/103f0369fa02aac21aae282e4f7f81c903aba6be/uniqueidentifier.go#L78

taniabogatsch commented 4 months ago

It is more of a convenient way to directly serialize results obtained from the library to JSON/string. I can add it at application level as well but thought it will be useful to add it directly to the original type since similar function also exists for big.Float?

After discussing this with @marcboeker, we prefer to keep this on the application level (for now). We've enabled the Discussions feature in the repository, however, and you could move it there, to discuss it as an idea. If there is a lot of traction around more features for currently exposed custom types, we can reiterate on it?

k-anshul commented 4 months ago

Sure. I can add the method in the application for now. We can iterate on this in future. I am closing this PR for now. Thanks for the inputs.