From 62f55164f7afe7bad4c7443bf85e5783738ecf03 Mon Sep 17 00:00:00 2001 From: Timothy Trindle <48774137+titrindl@users.noreply.github.com> Date: Tue, 24 Sep 2024 15:49:46 -0700 Subject: [PATCH] add new op type for sided obliterates (#22596) This PR introduces the new `MergeTreeDeltaType.OBLITERATE_SIDED`, which uses objects for `pos1` and `pos2` with side information. This is necessary for obliterate endpoint expansion, which will be implemented in a follow-up PR. See #22552 for a functional prototype of endpoint expansion. --- .../api-report/merge-tree.legacy.alpha.api.md | 23 +++++- packages/dds/merge-tree/package.json | 15 +++- packages/dds/merge-tree/src/client.ts | 79 ++++++++++++++----- packages/dds/merge-tree/src/index.ts | 1 + packages/dds/merge-tree/src/opBuilder.ts | 32 ++++++++ packages/dds/merge-tree/src/ops.ts | 24 +++++- .../merge-tree/src/test/reconnectHelper.ts | 3 +- .../validateMergeTreePrevious.generated.ts | 4 + packages/dds/sequence/package.json | 15 +++- .../validateSequencePrevious.generated.ts | 4 + 10 files changed, 175 insertions(+), 25 deletions(-) diff --git a/packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md b/packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md index 6d25ff9949e7..c88e49b11511 100644 --- a/packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md +++ b/packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md @@ -144,7 +144,7 @@ export class Client extends TypedEventEmitter { readonly logger: ITelemetryLoggerExt; // (undocumented) longClientId: string | undefined; - obliterateRangeLocal(start: number, end: number): IMergeTreeObliterateMsg; + obliterateRangeLocal(start: number | InteriorSequencePlace, end: number | InteriorSequencePlace): IMergeTreeObliterateMsg | IMergeTreeObliterateSidedMsg; peekPendingSegmentGroups(): SegmentGroup | undefined; // (undocumented) peekPendingSegmentGroups(count: number): SegmentGroup | SegmentGroup[] | undefined; @@ -320,7 +320,7 @@ export interface IMergeTreeDeltaCallbackArgs { * Obliterates the range. This is similar to removing the range, but also * includes any concurrently inserted content. * - * @param start - The inclusive start of the range to obliterate - * @param end - The exclusive end of the range to obliterate + * @param start - The start of the range to obliterate. Inclusive is side is Before (default). + * @param end - The end of the range to obliterate. Exclusive is side is After + * (default is to be after the last included character, but number index is exclusive). */ public obliterateRangeLocal( - start: number, - end: number, + start: number | InteriorSequencePlace, + end: number | InteriorSequencePlace, // eslint-disable-next-line import/no-deprecated - ): IMergeTreeObliterateMsg { - const obliterateOp = createObliterateRangeOp(start, end); + ): IMergeTreeObliterateMsg | IMergeTreeObliterateSidedMsg { + // eslint-disable-next-line import/no-deprecated + let obliterateOp: IMergeTreeObliterateMsg | IMergeTreeObliterateSidedMsg; + if (this._mergeTree.options?.mergeTreeEnableSidedObliterate) { + obliterateOp = createObliterateRangeOpSided(start, end); + } else { + assert( + typeof start === "number" && typeof end === "number", + "Start and end must be numbers if mergeTreeEnableSidedObliterate is not enabled.", + ); + obliterateOp = createObliterateRangeOp(start, end); + } this.applyObliterateRangeOp({ op: obliterateOp }); return obliterateOp; } @@ -472,22 +486,40 @@ export class Client extends TypedEventEmitter { private applyObliterateRangeOp(opArgs: IMergeTreeDeltaOpArgs): void { assert( - opArgs.op.type === MergeTreeDeltaType.OBLITERATE, + opArgs.op.type === MergeTreeDeltaType.OBLITERATE || + opArgs.op.type === MergeTreeDeltaType.OBLITERATE_SIDED, 0x866 /* Unexpected op type on range obliterate! */, ); const op = opArgs.op; const clientArgs = this.getClientSequenceArgs(opArgs); - const range = this.getValidOpRange(op, clientArgs); - - this._mergeTree.obliterateRange( - range.start, - range.end, - clientArgs.referenceSequenceNumber, - clientArgs.clientId, - clientArgs.sequenceNumber, - false, - opArgs, - ); + if (this._mergeTree.options?.mergeTreeEnableSidedObliterate) { + /* + const _start: InteriorSequencePlace = + typeof op.pos1 === "object" + ? { pos: op.pos1.pos, side: op.pos1.before ? Side.Before : Side.After } + : { pos: op.pos1, side: Side.Before }; + const _end: InteriorSequencePlace = + typeof op.pos2 === "object" + ? { pos: op.pos2.pos, side: op.pos2.before ? Side.Before : Side.After } + : { pos: op.pos2 - 1, side: Side.After }; + */ + assert(false, "TODO: sided obliterate will come in a follow-up PR shortly."); + } else { + assert( + op.type === MergeTreeDeltaType.OBLITERATE, + "Unexpected sided obliterate while mergeTreeEnableSidedObliterate is disabled", + ); + const range = this.getValidOpRange(op, clientArgs); + this._mergeTree.obliterateRange( + range.start, + range.end, + clientArgs.referenceSequenceNumber, + clientArgs.clientId, + clientArgs.sequenceNumber, + false, + opArgs, + ); + } } /** @@ -891,7 +923,12 @@ export class Client extends TypedEventEmitter { const first = opList[0]; - if (!!first && first.pos2 !== undefined) { + if ( + !!first && + first.pos2 !== undefined && + first.type !== MergeTreeDeltaType.OBLITERATE_SIDED && + newOp.type !== MergeTreeDeltaType.OBLITERATE_SIDED + ) { first.pos2 += newOp.pos2! - newOp.pos1!; } else { opList.push(newOp); @@ -976,6 +1013,10 @@ export class Client extends TypedEventEmitter { this.applyObliterateRangeOp({ op }); break; } + case MergeTreeDeltaType.OBLITERATE_SIDED: { + assert(false, "TODO: sided obliterate will come in a follow-up PR shortly."); + break; + } case MergeTreeDeltaType.GROUP: { op.ops.map((o) => this.applyStashedOp(o)); break; diff --git a/packages/dds/merge-tree/src/index.ts b/packages/dds/merge-tree/src/index.ts index efb5f843ff68..b16e5779c3ca 100644 --- a/packages/dds/merge-tree/src/index.ts +++ b/packages/dds/merge-tree/src/index.ts @@ -100,6 +100,7 @@ export { MergeTreeDeltaType, ReferenceType, IMergeTreeObliterateMsg, + IMergeTreeObliterateSidedMsg, } from "./ops.js"; export { addProperties, diff --git a/packages/dds/merge-tree/src/opBuilder.ts b/packages/dds/merge-tree/src/opBuilder.ts index d67ce55c3859..3cf18f16f7dd 100644 --- a/packages/dds/merge-tree/src/opBuilder.ts +++ b/packages/dds/merge-tree/src/opBuilder.ts @@ -14,8 +14,10 @@ import { IMergeTreeObliterateMsg, IMergeTreeRemoveMsg, MergeTreeDeltaType, + type IMergeTreeObliterateSidedMsg, } from "./ops.js"; import { PropertySet } from "./properties.js"; +import { normalizePlace, Side, type SequencePlace } from "./sequencePlace.js"; /** * Creates the op for annotating the markers with the provided properties @@ -97,6 +99,36 @@ export function createObliterateRangeOp(start: number, end: number): IMergeTreeO }; } +/** + * Creates the op to obliterate a range + * + * @param start - The start of the range to obliterate. + * If a number is provided, the range will start before that index. + * @param end - The end of the range to obliterate. + * If a number is provided, the range will end after that index -1. + * This preserves the previous behavior of not expanding obliteration ranges at the endpoints + * for uses which predate the availability of endpoint expansion. + * + * @internal + */ +export function createObliterateRangeOpSided( + start: SequencePlace, + end: SequencePlace, +): IMergeTreeObliterateSidedMsg { + const startPlace = normalizePlace(start); + // If a number is provided, default to after the previous index. + // This preserves the behavior of obliterate prior to the introduction of endpoint expansion. + const endPlace = + typeof end === "number" + ? { pos: end - 1, side: Side.After } // default to inclusive bounds + : normalizePlace(end); + return { + type: MergeTreeDeltaType.OBLITERATE_SIDED, + pos1: { pos: startPlace.pos, before: startPlace.side === Side.Before }, + pos2: { pos: endPlace.pos, before: endPlace.side === Side.Before }, + }; +} + /** * Creates an op for inserting a segment at the specified position. * diff --git a/packages/dds/merge-tree/src/ops.ts b/packages/dds/merge-tree/src/ops.ts index 1ce6b517750c..ccdc0b78a257 100644 --- a/packages/dds/merge-tree/src/ops.ts +++ b/packages/dds/merge-tree/src/ops.ts @@ -70,6 +70,7 @@ export const MergeTreeDeltaType = { */ GROUP: 3, OBLITERATE: 4, + OBLITERATE_SIDED: 5, } as const; /** @@ -162,6 +163,26 @@ export interface IMergeTreeObliterateMsg extends IMergeTreeDelta { relativePos2?: never; } +/** + * @legacy + * @alpha + */ +export interface IMergeTreeObliterateSidedMsg extends IMergeTreeDelta { + type: typeof MergeTreeDeltaType.OBLITERATE_SIDED; + pos1: { pos: number; before: boolean }; + /** + * This field is currently unused, but we keep it around to make the union + * type of all merge-tree messages have the same fields + */ + relativePos1?: never; + pos2: { pos: number; before: boolean }; + /** + * This field is currently unused, but we keep it around to make the union + * type of all merge-tree messages have the same fields + */ + relativePos2?: never; +} + /** * @legacy * @alpha @@ -206,7 +227,8 @@ export type IMergeTreeDeltaOp = | IMergeTreeInsertMsg | IMergeTreeRemoveMsg | IMergeTreeAnnotateMsg - | IMergeTreeObliterateMsg; + | IMergeTreeObliterateMsg + | IMergeTreeObliterateSidedMsg; /** * @legacy diff --git a/packages/dds/merge-tree/src/test/reconnectHelper.ts b/packages/dds/merge-tree/src/test/reconnectHelper.ts index 5b9a8bc8eaf2..5ccefec3786d 100644 --- a/packages/dds/merge-tree/src/test/reconnectHelper.ts +++ b/packages/dds/merge-tree/src/test/reconnectHelper.ts @@ -18,6 +18,7 @@ import { IMergeTreeDeltaOp, type IMergeTreeInsertMsg, type IMergeTreeObliterateMsg, + type IMergeTreeObliterateSidedMsg, type IMergeTreeRemoveMsg, } from "../ops.js"; @@ -121,7 +122,7 @@ export class ReconnectTestHelper { start: SequencePlace, end: SequencePlace, ): { - op: IMergeTreeObliterateMsg; + op: IMergeTreeObliterateMsg | IMergeTreeObliterateSidedMsg; seg: SegmentGroup; refSeq: number; } { diff --git a/packages/dds/merge-tree/src/test/types/validateMergeTreePrevious.generated.ts b/packages/dds/merge-tree/src/test/types/validateMergeTreePrevious.generated.ts index bea7d5d90d68..3e9155bac160 100644 --- a/packages/dds/merge-tree/src/test/types/validateMergeTreePrevious.generated.ts +++ b/packages/dds/merge-tree/src/test/types/validateMergeTreePrevious.generated.ts @@ -967,6 +967,7 @@ declare type old_as_current_for_Interface_IMergeTreeDeltaOpArgs = requireAssigna * typeValidation.broken: * "Interface_IMergeTreeDeltaOpArgs": {"backCompat": false} */ +// @ts-expect-error compatibility expected to be broken declare type current_as_old_for_Interface_IMergeTreeDeltaOpArgs = requireAssignableTo, TypeOnly> /* @@ -985,6 +986,7 @@ declare type old_as_current_for_Interface_IMergeTreeGroupMsg = requireAssignable * typeValidation.broken: * "Interface_IMergeTreeGroupMsg": {"backCompat": false} */ +// @ts-expect-error compatibility expected to be broken declare type current_as_old_for_Interface_IMergeTreeGroupMsg = requireAssignableTo, TypeOnly> /* @@ -1570,6 +1572,7 @@ declare type old_as_current_for_TypeAlias_IMergeTreeDeltaOp = requireAssignableT * typeValidation.broken: * "TypeAlias_IMergeTreeDeltaOp": {"backCompat": false} */ +// @ts-expect-error compatibility expected to be broken declare type current_as_old_for_TypeAlias_IMergeTreeDeltaOp = requireAssignableTo, TypeOnly> /* @@ -1588,6 +1591,7 @@ declare type old_as_current_for_TypeAlias_IMergeTreeOp = requireAssignableTo, TypeOnly> /* diff --git a/packages/dds/sequence/package.json b/packages/dds/sequence/package.json index 8adfb9f78646..7715a9e359c5 100644 --- a/packages/dds/sequence/package.json +++ b/packages/dds/sequence/package.json @@ -191,7 +191,20 @@ } }, "typeValidation": { - "broken": {}, + "broken": { + "ClassStatics_SequenceMaintenanceEvent": { + "backCompat": false + }, + "Class_SequenceMaintenanceEvent": { + "backCompat": false + }, + "ClassStatics_SequenceDeltaEvent": { + "backCompat": false + }, + "Class_SequenceDeltaEvent": { + "backCompat": false + } + }, "entrypoint": "internal" } } diff --git a/packages/dds/sequence/src/test/types/validateSequencePrevious.generated.ts b/packages/dds/sequence/src/test/types/validateSequencePrevious.generated.ts index 190c74417ebd..a53eaa31c251 100644 --- a/packages/dds/sequence/src/test/types/validateSequencePrevious.generated.ts +++ b/packages/dds/sequence/src/test/types/validateSequencePrevious.generated.ts @@ -85,6 +85,7 @@ declare type old_as_current_for_Class_SequenceDeltaEvent = requireAssignableTo, TypeOnly> /* @@ -139,6 +140,7 @@ declare type old_as_current_for_Class_SequenceMaintenanceEvent = requireAssignab * typeValidation.broken: * "Class_SequenceMaintenanceEvent": {"backCompat": false} */ +// @ts-expect-error compatibility expected to be broken declare type current_as_old_for_Class_SequenceMaintenanceEvent = requireAssignableTo, TypeOnly> /* @@ -319,6 +321,7 @@ declare type current_as_old_for_ClassStatics_Marker = requireAssignableTo, TypeOnly> /* @@ -346,6 +349,7 @@ declare type current_as_old_for_ClassStatics_SequenceInterval = requireAssignabl * typeValidation.broken: * "ClassStatics_SequenceMaintenanceEvent": {"backCompat": false} */ +// @ts-expect-error compatibility expected to be broken declare type current_as_old_for_ClassStatics_SequenceMaintenanceEvent = requireAssignableTo, TypeOnly> /*