Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array support with delimiter interface #466

Closed
wants to merge 10 commits into from
Closed

Conversation

cbandy
Copy link
Contributor

@cbandy cbandy commented May 26, 2016

This adds types that implement driver.Valuer and sql.Scanner for arrays and slices.

There are a handful of "native" types that efficiently handle one-dimensional slices (e.g. StringArray for []string) and a "generic" type that uses reflection to traverse multi-dimensional arrays/slices.

The ArrayDelimiter interface can be implemented by a type that does not use comma as its delimiter. For example,

type Box struct { ... }
func (b Box) Value() (driver.Value, error) { ... }
func (b Box) ArrayDelimiter() string { return ";" }

The most convenient way to use these types is probably the Array function. It provides a consistent way to send and receive arrays and slices of various types. For example,

db.Query(..., pq.Array([]string{ ... }))
db.Query(..., pq.Array([][]int{ ... }))

var x []sql.NullInt64
var y []string
db.QueryRow(...).Scan(pq.Array(&x))
db.QueryRow(...).Scan(pq.Array(&y))

See also #49, #130, #327, #353, #364, #464.

cbandy added 10 commits May 26, 2016 02:33
The backend process for interpreting text-format array input uses this
delimiter and the text-format input routines of the element type.
These are faster and less memory-intensive than the reflection-based
Valuer.
To be compatible with older versions of PostgreSQL, use escape string
syntax for values containing backslash.
For example, UUID is often implemented as an array of bytes. Such an
array should be treated as a value rather than another dimension.
@cbandy
Copy link
Contributor Author

cbandy commented May 26, 2016

GenericArray.Scan() is artificially limited to handle only one-dimensional array/slice of sql.Scanner as discussed toward the end of #364. I simply ran out of time to handle all the possible combinations of types and dimensions. Fortunately, those limitations can be lifted (gradually, even) without changing the exported API in any way.

@freeformz
Copy link

@johto Any thoughts on this?

@johto
Copy link
Contributor

johto commented Jun 4, 2016

I'll have a closer look at this over the weekend.

My memory isn't that great, but I thought there were serious issues discussed with these types of struct definitions in #364, which is why I suggested doing:

type Int64Array struct { A *[]int64 }

instead. Have those issues been fixed here?

@cbandy
Copy link
Contributor Author

cbandy commented Jun 4, 2016

My memory isn't that great, but I thought there were serious issues discussed with these types of struct definitions

Have those issues been fixed here?

I think you're referring to things implementing both Scanner and Valuer. The Array function returns pointers to achieve this, e.g. return (*BoolArray)(&a). The GenericArray struct behaves as you've described.

Yes, I'd say those issues have been fixed.

@swsnider
Copy link

swsnider commented Aug 2, 2016

Any chance of this getting merged soon? I'm going to have to hold this patch in a private fork otherwise :(.

@ppeble
Copy link

ppeble commented Aug 5, 2016

+1 on merging. We would like to use this improvement in our internal projects.

@johto
Copy link
Contributor

johto commented Aug 6, 2016

OK, I'm working on this right now.

@johto
Copy link
Contributor

johto commented Aug 6, 2016

I've pushed this in 80f8150 after a number of changes. Most notably, I changed code like this:

func whatever(...) (err error) {
    elems, err := scanLinearArray(src, []byte{','}, "ByteaArray")

    switch {
    case err != nil:
        break

because it set off the "unintended shadowing" bells in my head really strongly. The spec says the "err" here redeclares the one in the parameter list of the result specification of the function, but it's not obvious to the reader -- I had to literally open the spec to see what the behavior should be, and I consider myself fairly experienced as a Go developer. I also changed a number of if statements masquerading as switches. Some of them were hurting readability seriously, some of them just felt icky.

I'm also really not happy with the way parseArray is written right now. It's a big non-obvious mess of goto spaghetti, and I had to read several blocks of it more than three times to understand what was really going on. It should be rewritten using recursion instead (perhaps passing around a small struct with the parse state), but I didn't do that now. I have other things to do today.

I also added documentation about the lack of support for scanning of multi-dimensional arrays and arrays with unorthodox bounds.

@ppeble
Copy link

ppeble commented Aug 6, 2016

Thank you @johto!

@cbandy
Copy link
Contributor Author

cbandy commented Aug 6, 2016

@johto, thank you for reviewing and merging this. A lot of people expressed interest, but you're the only one to comment on the actual patches here. Thank you for taking care of the issues with shadowing and readability.

It wasn't my intention to leave you with code that you find hard to maintain. I want my contributions to this project to positive for both users and maintainers alike. I'm sorry for leaving this branch open for so long and I'm sorry if I haven't seemed open to feedback.

Also, thank you to @erykwalder. This is absolutely based on your work in #353 I had forgotten how many of your suggestions in #364 I had incorporated here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants