Skip to content

feat(schema, data-modeling): use iterable cursor in schema analysis COMPASS-9150 COMPASS-9315 #6894

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 8, 2025
Merged

feat(schema, data-modeling): use iterable cursor in schema analysis COMPASS-9150 COMPASS-9315 #6894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e47e498
fix(compass-schema): update schema analysis to use iterable cursor CO…
Anemy May 5, 2025
604903e
remove comment
Anemy May 5, 2025
4fed9a1
fixup: update schema-validation data service usage to use cursor
Anemy May 5, 2025
65b8b59
fixup: improved generator typing
Anemy May 7, 2025
3e8cff7
fixup: add abort reason
Anemy May 7, 2025
fe85960
fixup: use message from reason when available
Anemy May 7, 2025
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
2 changes: 2 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/compass-data-modeling/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
"compass-preferences-model": "^2.35.0",
"hadron-app-registry": "^9.4.8",
"lodash": "^4.17.21",
"mongodb": "^6.14.1",
"mongodb-ns": "^2.4.2",
"mongodb-schema": "^12.5.2",
"react": "^17.0.2",
Expand Down
32 changes: 17 additions & 15 deletions packages/compass-data-modeling/src/store/analysis-process.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { isAction } from './util';
import type { DataModelingThunkAction } from './reducer';
import { analyzeDocuments } from 'mongodb-schema';
import { getCurrentDiagramFromState } from './diagram';
import type { Document } from 'bson';
import type { AggregationCursor } from 'mongodb';

export type AnalysisProcessState = {
currentAnalysisOptions:
Expand Down Expand Up @@ -155,32 +157,32 @@ export function startAnalysis(
try {
const dataService =
services.connections.getDataServiceForConnection(connectionId);
const samples = await Promise.all(
const schema = await Promise.all(
namespaces.map(async (ns) => {
// TODO
const sample = await dataService.sample(
const sample: AggregationCursor<Document> = dataService.sampleCursor(
ns,
{ size: 100 },
undefined,
{
abortSignal: cancelController.signal,
signal: cancelController.signal,
promoteValues: false,
},
{
fallbackReadPreference: 'secondaryPreferred',
}
);

const accessor = await analyzeDocuments(sample, {
signal: cancelController.signal,
});

// TODO(COMPASS-9314): Update how we show analysis progress.
dispatch({
type: AnalysisProcessActionTypes.NAMESPACE_SAMPLE_FETCHED,
namespace: ns,
});
return { ns, sample };
})
);
const schema = await Promise.all(
samples.map(async ({ ns, sample }) => {
const schema = await analyzeDocuments(sample, {

const schema = await accessor.getMongoDBJsonSchema({
signal: cancelController.signal,
}).then((accessor) => {
return accessor.getMongoDBJsonSchema({
signal: cancelController.signal,
});
});
dispatch({
type: AnalysisProcessActionTypes.NAMESPACE_SCHEMA_ANALYZED,
Expand Down
10 changes: 3 additions & 7 deletions packages/compass-schema-validation/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { createLoggerLocator } from '@mongodb-js/compass-logging/provider';
import { telemetryLocator } from '@mongodb-js/compass-telemetry/provider';
import { SchemaValidationTabTitle } from './plugin-title';
import { workspacesServiceLocator } from '@mongodb-js/compass-workspaces/provider';
import type { RequiredDataServiceProps } from './modules';

const CompassSchemaValidationHadronPlugin = registerHadronPlugin(
{
Expand All @@ -23,13 +24,8 @@ const CompassSchemaValidationHadronPlugin = registerHadronPlugin(
activate: onActivated,
},
{
dataService: dataServiceLocator as DataServiceLocator<
| 'aggregate'
| 'collectionInfo'
| 'updateCollection'
| 'sample'
| 'isCancelError'
>,
dataService:
dataServiceLocator as DataServiceLocator<RequiredDataServiceProps>,
connectionInfoRef: connectionInfoRefLocator,
instance: mongoDBInstanceLocator,
preferences: preferencesLocator,
Expand Down
9 changes: 4 additions & 5 deletions packages/compass-schema-validation/src/modules/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,13 @@ export type RootAction =
| EditModeAction
| ResetAction;

export type DataService = Pick<
OriginalDataService,
export type RequiredDataServiceProps =
| 'aggregate'
| 'collectionInfo'
| 'updateCollection'
| 'sample'
| 'isCancelError'
>;
| 'sampleCursor'
| 'isCancelError';
export type DataService = Pick<OriginalDataService, RequiredDataServiceProps>;

export type SchemaValidationExtraArgs = {
dataService: DataService;
Expand Down
8 changes: 7 additions & 1 deletion packages/compass-schema-validation/src/stores/store.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ const fakeDataService = {
/* never resolves */
}),
isCancelError: () => false,
sample: () => [{ prop1: 'abc' }],
sampleCursor: () =>
({
async *[Symbol.asyncIterator]() {
await new Promise((resolve) => setTimeout(resolve, 0));
yield* [{ prop1: 'abc' }];
},
} as any),
} as any;

const fakeWorkspaces = {
Expand Down
6 changes: 3 additions & 3 deletions packages/compass-schema/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import CompassSchema from './components/compass-schema';
import { registerHadronPlugin } from 'hadron-app-registry';
import { activateSchemaPlugin } from './stores/store';
import type { RequiredDataServiceProps } from './stores/store';
import { createLoggerLocator } from '@mongodb-js/compass-logging/provider';
import { telemetryLocator } from '@mongodb-js/compass-telemetry/provider';
import { preferencesLocator } from 'compass-preferences-model/provider';
Expand All @@ -31,9 +32,8 @@ const CompassSchemaHadronPlugin = registerHadronPlugin(
activate: activateSchemaPlugin,
},
{
dataService: dataServiceLocator as DataServiceLocator<
'sample' | 'isCancelError'
>,
dataService:
dataServiceLocator as DataServiceLocator<RequiredDataServiceProps>,
logger: createLoggerLocator('COMPASS-SCHEMA-UI'),
track: telemetryLocator,
preferences: preferencesLocator,
Expand Down
72 changes: 51 additions & 21 deletions packages/compass-schema/src/modules/schema-analysis.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import mongoDBSchemaAnalyzeSchema from 'mongodb-schema';
import type { Schema } from 'mongodb-schema';
import { createNoopLogger } from '@mongodb-js/compass-logging/provider';
import { isInternalFieldPath } from 'hadron-document';

import { analyzeSchema, calculateSchemaMetadata } from './schema-analysis';
import {
createSandboxFromDefaultPreferences,
type PreferencesAccess,
} from 'compass-preferences-model';

import { analyzeSchema, calculateSchemaMetadata } from './schema-analysis';

const testDocs = [
{
someFields: {
Expand Down Expand Up @@ -81,13 +81,18 @@ describe('schema-analysis', function () {

describe('#getResult', function () {
it('returns the schema', async function () {
const docs = [
{ x: 1 },
{ y: 2, __safeContent__: [bson.Binary.createFromBase64('aaaa')] },
];
const dataService = {
sample: () =>
Promise.resolve([
{ x: 1 },
{ y: 2, __safeContent__: [bson.Binary.createFromBase64('aaaa')] },
]),
isCancelError: () => false,
sampleCursor: () =>
({
async *[Symbol.asyncIterator]() {
await new Promise((resolve) => setTimeout(resolve, 0));
yield* docs;
},
} as any),
};
const abortController = new AbortController();
const abortSignal = abortController.signal;
Expand Down Expand Up @@ -179,10 +184,17 @@ describe('schema-analysis', function () {

it('adds promoteValues: false so the analyzer can report more accurate types', async function () {
const dataService = {
sample: () => Promise.resolve([]),
isCancelError: () => false,
sampleCursor: () =>
({
async *[Symbol.asyncIterator]() {
await new Promise((resolve) => setTimeout(resolve, 0));
yield {
a: 123,
};
},
} as any),
};
const sampleSpy = sinon.spy(dataService, 'sample');
const sampleSpy = sinon.spy(dataService, 'sampleCursor');
const abortController = new AbortController();
const abortSignal = abortController.signal;

Expand All @@ -199,19 +211,31 @@ describe('schema-analysis', function () {
expect(sampleSpy).to.have.been.calledWith(
'db.coll',
{},
{ promoteValues: false }
{ signal: abortSignal, promoteValues: false },
{ fallbackReadPreference: 'secondaryPreferred' }
);
});

it('returns undefined if is cancelled', async function () {
const dataService = {
sample: () => Promise.reject(new Error('test error')),
isCancelError: () => true,
};

const abortController = new AbortController();
const abortSignal = abortController.signal;

const dataService = {
sampleCursor: () =>
({
async *[Symbol.asyncIterator]() {
await new Promise((resolve) => setTimeout(resolve, 0));
yield {
a: 123,
};
abortController.abort();
yield {
a: 345,
};
},
} as any),
};

const result = await analyzeSchema(
dataService,
abortSignal,
Expand All @@ -228,13 +252,19 @@ describe('schema-analysis', function () {
it('throws if sample throws', async function () {
const error: Error & {
code?: any;
} = new Error('should have been thrown');
} = new Error('pineapple');
error.name = 'MongoError';
error.code = new bson.Int32(1000);

const dataService = {
sample: () => Promise.reject(error),
isCancelError: () => false,
sampleCursor: () =>
({
async *[Symbol.asyncIterator]() {
await new Promise((resolve) => setTimeout(resolve, 0));
yield {};
throw error;
},
} as any),
};

const abortController = new AbortController();
Expand All @@ -251,7 +281,7 @@ describe('schema-analysis', function () {
preferences
);
} catch (err: any) {
expect(err.message).to.equal('should have been thrown');
expect(err.message).to.equal('pineapple');
expect(err.code).to.equal(1000);
return;
}
Expand Down
17 changes: 13 additions & 4 deletions packages/compass-schema/src/modules/schema-analysis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ export const analyzeSchema = async (
ns,
});

const docs = await dataService.sample(
const sampleCursor = dataService.sampleCursor(
ns,
query,
{
...aggregateOptions,
promoteValues: false,
signal: abortSignal,
},
{
abortSignal,
fallbackReadPreference: 'secondaryPreferred',
}
);
Expand All @@ -72,7 +72,10 @@ export const analyzeSchema = async (
: {
signal: abortSignal,
};
const schemaAccessor = await analyzeDocuments(docs, schemaParseOptions);
const schemaAccessor = await analyzeDocuments(
sampleCursor,
Copy link
Member Author

@Anemy Anemy May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This could impact performance. I have not tested that. Should we?

Copy link
Collaborator

@nbbeeken nbbeeken May 5, 2025

Choose a reason for hiding this comment

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

Any specific concerns or predictions for where the biggest impact might be? I def defer to another team member to give you an answer here, but as a newbie what would it entail to validate at least the biggest bottlenecks we think may come up here?

Copy link
Member Author

@Anemy Anemy May 7, 2025

Choose a reason for hiding this comment

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

Now we're checking if the signal is aborted between every document parsing. I don't think that would be any overhead. The part I think something could change is just that we don't do the toArray and then pass all of the documents synchronously, now the schema analysis will wait longer if there are any round trips. I don't think that would slow things down either really, but I wanted to raise it to make sure it's something on our minds.

Copy link
Collaborator

@nbbeeken nbbeeken May 7, 2025

Choose a reason for hiding this comment

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

Sorry didn't get back sooner. I agree, we've shifted the cost but in a way that should leave the app a bit more responsive as it acts on a document at a time. Plus this takes advantage of the driver's lazy deserialize. In the end the time spent should be the equivalent, broken up over more event loop cycles

schemaParseOptions
);
log.info(mongoLogId(1001000090), 'Schema', 'Schema analysis completed', {
ns,
});
Expand All @@ -81,8 +84,14 @@ export const analyzeSchema = async (
log.error(mongoLogId(1001000091), 'Schema', 'Schema analysis failed', {
ns,
error: err.message,
aborted: abortSignal.aborted,
...(abortSignal.aborted
? { abortReason: abortSignal.reason?.message ?? abortSignal.reason }
: {}),
});
if (dataService.isCancelError(err)) {

if (abortSignal.aborted) {
// The operation was aborted, so we don't throw an error.
debug('caught background operation terminated error', err);
return;
}
Expand Down
Loading
Loading