Implement proper array-like SQL record/row compatible with 6b1 #1156
Conversation
| case CTYPE_RECORD: { | ||
| const els = frb.readUInt16(); | ||
| for (let i = 0; i < els; i++) { | ||
| const elm_length = frb.readUInt32(); | ||
| frb.discard(elm_length + 2); | ||
| } | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this code will ever actually get executed, since for protocol v2+ the type descriptors are length prefixed, so we skip the whole typedesc in one go (line 113).
There was a problem hiding this comment.
Hm, good point, I'll double check
| result[i] = val; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
I thought we were going to decode sql records into some custom datatype where you could access columns by name or index?
There was a problem hiding this comment.
I'm pesonally fine with this being an array of tuples by default. If we had a custom datatype for this here are some options I've just thought of:
asTuplesandasObjectsmethods to make it easy to serialize, but then we pay for the overhead of the custom datatype.toJSONmethod that serializes as an array of tuples- wrap an array of tuples (and has the appropriate getter in the
valueOfprototype method) and provide awithNamesmethod that will wrap it in an object interface with the names
There was a problem hiding this comment.
I thought so too and then I started digging why we stopped doing custom data types for named tuples and found this: #310
tl;dr: we can do custom types but some JS frameworks hardcode their JSON serialization exclusively for Array / Object types and fail with anything that extends them or does anything funky
There was a problem hiding this comment.
Yeah, I support just making this an array of tuples and worry about exposing a way of getting an array of objects as a separate effort.
There was a problem hiding this comment.
Yeah, I had forgot about the serialisation issues. Maybe this can be solved with the custom codecs api? We could default to decoding to arrays as it is now, but also provide a builtin 'custom' codec that would decode to objects with names, so users could choose?
There was a problem hiding this comment.
provide a builtin 'custom' codec that would decode to objects with names, so users could choose?
Oh, yes! This was my original idea, but I had forgotten that I had it 😆 🙈
There was a problem hiding this comment.
Yeah, I thought about this too. It might be a slightly different special cased version of .withCodecs, because "SQL Record" doesn't really have a type name. Can be symbol though...
No description provided.