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

Implement proper array-like SQL record/row compatible with 6b1 #1156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/driver/src/codecs/ifaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ export type CodecKind =
| "scalar"
| "sparse_object"
| "range"
| "multirange";
| "multirange"
| "record";

export interface ICodec {
readonly tid: uuid;
Expand Down
3 changes: 2 additions & 1 deletion packages/driver/src/codecs/object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ export class ObjectCodec extends Codec implements ICodec {
}

encodeArgs(args: any): Uint8Array {
if (this.fields[0].name === "0") {
// EdgeQL query parameters start at 0, SQL start at 1.
if (this.fields[0].name === "0" || this.fields[0].name === "1") {
return this._encodePositionalArgs(args);
}
return this._encodeNamedArgs(args);
Expand Down
84 changes: 84 additions & 0 deletions packages/driver/src/codecs/record.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*!
* This source file is part of the EdgeDB open source project.
*
* Copyright 2019-present MagicStack Inc. and the EdgeDB authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import type { ICodec, uuid, CodecKind } from "./ifaces";
import { Codec } from "./ifaces";
import { ReadBuffer, WriteBuffer } from "../primitives/buffer";

Check failure on line 21 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (18, ubuntu-latest, stable)

Imports "WriteBuffer" are only used as type

Check failure on line 21 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (20, ubuntu-latest, stable)

Imports "WriteBuffer" are only used as type

Check failure on line 21 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (22, ubuntu-latest, stable)

Imports "WriteBuffer" are only used as type

Check failure on line 21 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, nightly)

Imports "WriteBuffer" are only used as type

Check failure on line 21 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, 3)

Imports "WriteBuffer" are only used as type

Check failure on line 21 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, 2)

Imports "WriteBuffer" are only used as type
import {
InvalidArgumentError,
ProtocolError,
} from "../errors";

export class RecordCodec extends Codec implements ICodec {
private subCodecs: ICodec[];
private names: string[];

constructor(
tid: uuid,
codecs: ICodec[],
names: string[],
) {
super(tid);
this.subCodecs = codecs;
this.names = names;
}

encode(buf: WriteBuffer, object: any): void {

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (18, ubuntu-latest, stable)

'buf' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (18, ubuntu-latest, stable)

'object' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (20, ubuntu-latest, stable)

'buf' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (20, ubuntu-latest, stable)

'object' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (22, ubuntu-latest, stable)

'buf' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (22, ubuntu-latest, stable)

'object' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, nightly)

'buf' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, nightly)

'object' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, 3)

'buf' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, 3)

'object' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, 2)

'buf' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 41 in packages/driver/src/codecs/record.ts

View workflow job for this annotation

GitHub Actions / test (ubuntu-latest, 20, 2)

'object' is defined but never used. Allowed unused args must match /^_/u
throw new InvalidArgumentError(
"SQL records cannot be passed as arguments");
}

decode(buf: ReadBuffer): any {
const els = buf.readUInt32();
const subCodecs = this.subCodecs;
if (els !== subCodecs.length) {
throw new ProtocolError(
`cannot decode Record: expected ` +
`${subCodecs.length} elements, got ${els}`,
);
}

const elemBuf = ReadBuffer.alloc();
const result: any[] = new Array(els);
for (let i = 0; i < els; i++) {
buf.discard(4); // reserved
const elemLen = buf.readInt32();
let val = null;
if (elemLen !== -1) {
buf.sliceInto(elemBuf, elemLen);
val = subCodecs[i].decode(elemBuf);
elemBuf.finish();
}
result[i] = val;
}

return result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were going to decode sql records into some custom datatype where you could access columns by name or index?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • asTuples and asObjects methods to make it easy to serialize, but then we pay for the overhead of the custom datatype.
  • toJSON method that serializes as an array of tuples
  • wrap an array of tuples (and has the appropriate getter in the valueOf prototype method) and provide a withNames method that will wrap it in an object interface with the names

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆 🙈

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

}

getSubcodecs(): ICodec[] {
return Array.from(this.subCodecs);
}

getNames(): string[] {
return Array.from(this.names);
}

getKind(): CodecKind {
return "record";
}
}
30 changes: 30 additions & 0 deletions packages/driver/src/codecs/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { NamedTupleCodec } from "./namedtuple";
import { EnumCodec } from "./enum";
import { ObjectCodec } from "./object";
import { SetCodec } from "./set";
import { RecordCodec } from "./record";
import { MultiRangeCodec, RangeCodec } from "./range";
import type { ProtocolVersion } from "../ifaces";
import { versionGreaterThanOrEqual } from "../utils";
Expand All @@ -52,6 +53,7 @@ const CTYPE_RANGE = 9;
const CTYPE_OBJECT = 10;
const CTYPE_COMPOUND = 11;
const CTYPE_MULTIRANGE = 12;
const CTYPE_RECORD = 13;

export interface CustomCodecSpec {
int64_bigint?: boolean;
Expand Down Expand Up @@ -261,6 +263,15 @@ export class CodecsRegistry {
break;
}

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;
}

Comment on lines +266 to +274
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good point, I'll double check

case CTYPE_ARRAY: {
frb.discard(2);
const els = frb.readUInt16();
Expand Down Expand Up @@ -567,6 +578,25 @@ export class CodecsRegistry {
break;
}

case CTYPE_RECORD: {
const els = frb.readUInt16();
const codecs = new Array(els);
const names = new Array(els);
for (let i = 0; i < els; i++) {
names[i] = frb.readString();
const pos = frb.readUInt16();
const subCodec = cl[pos];
if (subCodec == null) {
throw new ProtocolError(
"could not build record codec: missing subcodec",
);
}
codecs[i] = subCodec;
}
res = new RecordCodec(tid, codecs, names);
break;
}

case CTYPE_ENUM: {
let typeName: string | null = null;
if (isProtoV2) {
Expand Down
10 changes: 5 additions & 5 deletions packages/driver/test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2366,13 +2366,13 @@ if (getEdgeDBVersion().major >= 6) {

try {
let res = await client.querySQL("select 1");
expect(JSON.stringify(res)).toEqual('[{"col~1":1}]');
expect(JSON.stringify(res)).toEqual('[[1]]');

res = await client.querySQL("select 1 AS foo, 2 AS bar");
expect(JSON.stringify(res)).toEqual('[{"foo":1,"bar":2}]');
expect(JSON.stringify(res)).toEqual('[[1,2]]');

res = await client.querySQL("select 1 + $1::int8", [41]);
expect(JSON.stringify(res)).toEqual('[{"col~1":42}]');
expect(JSON.stringify(res)).toEqual('[[42]]');
} finally {
await client.close();
}
Expand Down Expand Up @@ -2439,11 +2439,11 @@ if (getEdgeDBVersion().major >= 6) {

try {
for (const [typename, val] of pgTypes) {
const res = await client.querySQL<{ val: any }>(
const res = await client.querySQL<any>(
`select $1::${typename} as "val"`,
[val],
);
expect(JSON.stringify(res[0].val)).toEqual(JSON.stringify(val));
expect(JSON.stringify(res[0][0])).toEqual(JSON.stringify(val));
}
} finally {
await client.close();
Expand Down
Loading