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

Remove lodash dependency from @mcap/core #597

Merged
merged 3 commits into from
Sep 22, 2022
Merged
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
27 changes: 24 additions & 3 deletions typescript/core/src/pre0/parse.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import { isEqual } from "lodash";

import { getBigUint64 } from "../common/getBigUint64";
import { MCAP_MAGIC, RecordType } from "./constants";
import { McapMagic, McapRecord, ChannelInfo } from "./types";
Expand Down Expand Up @@ -122,7 +120,7 @@ export function parseRecord(
channelInfosSeenInThisChunk.add(id);
const existingInfo = channelInfosById.get(id);
if (existingInfo) {
if (!isEqual(existingInfo, record)) {
if (!isChannelInfoEqual(existingInfo, record)) {
throw new Error(`differing channel infos for ${record.id}`);
}
return {
Expand Down Expand Up @@ -191,3 +189,26 @@ export function parseRecord(
throw new Error("Not yet implemented");
}
}

function isChannelInfoEqual(a: ChannelInfo, b: ChannelInfo): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar thoughts to the channel info comparison below - maybe a crc on the bytes would be effective and work if we ever add a new field.

if (
a.data.byteLength !== b.data.byteLength ||
a.encoding !== b.encoding ||
a.id !== b.id ||
a.schema !== b.schema ||
a.schemaName !== b.schemaName ||
a.topic !== b.topic
) {
return false;
}

const aData = new Uint8Array(a.data);
const bData = new Uint8Array(b.data);
for (let i = 0; i < aData.length; i++) {
if (aData[i] !== bData[i]) {
return false;
}
}

return true;
}
14 changes: 3 additions & 11 deletions typescript/core/src/v0/ChunkCursor.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Heap from "heap-js";
import { sortedIndexBy } from "lodash";

import { parseRecord } from "./parse";
import { sortedIndexBy } from "./sortedIndexBy";
import { IReadable, TypedMcapRecords } from "./types";

type ChunkCursorParams = {
Expand Down Expand Up @@ -234,19 +234,11 @@ export class ChunkCursor {
let startIndex = 0;
if (reverse) {
if (this.endTime != undefined) {
startIndex = sortedIndexBy(
result.record.records,
[this.endTime],
([logTime]) => -logTime!,
);
startIndex = sortedIndexBy(result.record.records, this.endTime, (logTime) => -logTime);
}
} else {
if (this.startTime != undefined) {
startIndex = sortedIndexBy(
result.record.records,
[this.startTime],
([logTime]) => logTime,
);
startIndex = sortedIndexBy(result.record.records, this.startTime, (logTime) => logTime);
}
}

Expand Down
32 changes: 29 additions & 3 deletions typescript/core/src/v0/Mcap0StreamReader.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { crc32 } from "@foxglove/crc";
import { isEqual } from "lodash";

import StreamBuffer from "../common/StreamBuffer";
import { MCAP0_MAGIC } from "./constants";
import { parseMagic, parseRecord } from "./parse";
import { DecompressHandlers, McapStreamReader, TypedMcapRecord, TypedMcapRecords } from "./types";
import {
Channel,
DecompressHandlers,
McapStreamReader,
TypedMcapRecord,
TypedMcapRecords,
} from "./types";

type McapReaderOptions = {
/**
Expand Down Expand Up @@ -99,7 +104,7 @@ export default class Mcap0StreamReader implements McapStreamReader {
if (result.value?.type === "Channel") {
const existing = this.channelsById.get(result.value.id);
this.channelsById.set(result.value.id, result.value);
if (existing && !isEqual(existing, result.value)) {
if (existing && !isChannelEqual(existing, result.value)) {
throw new Error(
`Channel record for id ${result.value.id} (topic: ${result.value.topic}) differs from previous channel record of the same id.`,
);
Expand Down Expand Up @@ -258,3 +263,24 @@ export default class Mcap0StreamReader implements McapStreamReader {
}
}
}

function isChannelEqual(a: Channel, b: Channel): boolean {
if (
!(
a.id === b.id &&
a.messageEncoding === b.messageEncoding &&
a.schemaId === b.schemaId &&
a.topic === b.topic &&
a.metadata.size === b.metadata.size
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything we can do to protect against adding fields but forgetting to define them here for equality comparison?

Random thought: might it be better to store a crc of the original record bytes and compare that?

)
) {
return false;
}
for (const [keyA, valueA] of a.metadata.entries()) {
const valueB = b.metadata.get(keyA);
if (valueA !== valueB) {
return false;
}
}
return true;
}
53 changes: 53 additions & 0 deletions typescript/core/src/v0/sortedIndexBy.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import { sortedIndexBy } from "./sortedIndexBy";

describe("sortedIndexBy", () => {
it("handles an empty array", () => {
const array: [bigint, bigint][] = [];

expect(sortedIndexBy(array, 0n, (x) => x)).toEqual(0);
expect(sortedIndexBy(array, 42n, (x) => x)).toEqual(0);
});

it("handles a contiguous array", () => {
const array: [bigint, bigint][] = [
[1n, 42n],
[2n, 42n],
[3n, 42n],
];

expect(sortedIndexBy(array, 0n, (x) => x)).toEqual(0);
expect(sortedIndexBy(array, 1n, (x) => x)).toEqual(0);
expect(sortedIndexBy(array, 2n, (x) => x)).toEqual(1);
expect(sortedIndexBy(array, 3n, (x) => x)).toEqual(2);
expect(sortedIndexBy(array, 4n, (x) => x)).toEqual(3);
});

it("handles a sparse array", () => {
const array: [bigint, bigint][] = [
[1n, 42n],
[3n, 42n],
];

expect(sortedIndexBy(array, 0n, (x) => x)).toEqual(0);
expect(sortedIndexBy(array, 1n, (x) => x)).toEqual(0);
expect(sortedIndexBy(array, 2n, (x) => x)).toEqual(1);
expect(sortedIndexBy(array, 3n, (x) => x)).toEqual(1);
expect(sortedIndexBy(array, 4n, (x) => x)).toEqual(2);
});

it("handles negation", () => {
const array: [bigint, bigint][] = [
Copy link
Member

Choose a reason for hiding this comment

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

This test seems invalid? The input array is not sorted by (x)=>-x.

[1n, 42n],
[2n, 42n],
[3n, 42n],
[4n, 42n],
];

expect(sortedIndexBy(array, 0n, (x) => -x)).toEqual(4);
expect(sortedIndexBy(array, 1n, (x) => -x)).toEqual(4);
expect(sortedIndexBy(array, 2n, (x) => -x)).toEqual(4);
expect(sortedIndexBy(array, 3n, (x) => -x)).toEqual(0);
expect(sortedIndexBy(array, 4n, (x) => -x)).toEqual(0);
expect(sortedIndexBy(array, 5n, (x) => -x)).toEqual(0);
});
});
29 changes: 29 additions & 0 deletions typescript/core/src/v0/sortedIndexBy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Return the lowest index of `array` where an element can be inserted and maintain its sorted
* order. This is a specialization of lodash's sortedIndexBy().
*/
export function sortedIndexBy(
array: [bigint, bigint][],
value: bigint,
iteratee: (value: bigint) => bigint,
): number {
let low = 0;
let high = array.length;
if (high === 0) {
return 0;
}

const computedValue = iteratee(value);

while (low < high) {
const mid = (low + high) >>> 1;
Copy link
Member

Choose a reason for hiding this comment

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

The other math adjustment I was referring to was low + (high - low) >>>1, to avoid overflow during addition

const curComputedValue = iteratee(array[mid]![0]);

if (curComputedValue < computedValue) {
low = mid + 1;
} else {
high = mid;
}
}
return high;
}