[BUG] TS vector config source key should accept Key #5840
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Allow This minor bug-fix PR extends TypeScript definitions and runtime handling so that Key Changes• Updated Affected Areas• This summary was automatically generated by @propel-code-bot |
This comment has been minimized.
This comment has been minimized.
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| this.sourceKey = options.sourceKey instanceof Key ? options.sourceKey.name : options.sourceKey ?? null; | ||
| this.hnsw = options.hnsw ?? null; | ||
| this.spann = options.spann ?? null; | ||
| } | ||
| } | ||
|
|
||
| export interface SparseVectorIndexConfigOptions { | ||
| embeddingFunction?: SparseEmbeddingFunction | null; | ||
| sourceKey?: string | null; | ||
| sourceKey?: string | Key | null; | ||
| bm25?: boolean | null; | ||
| } | ||
|
|
||
| export class SparseVectorIndexConfig { | ||
| readonly type = "SparseVectorIndexConfig"; | ||
| embeddingFunction?: SparseEmbeddingFunction | null; | ||
| sourceKey: string | null; | ||
| bm25: boolean | null; | ||
|
|
||
| constructor(options: SparseVectorIndexConfigOptions = {}) { | ||
| this.embeddingFunction = options.embeddingFunction; | ||
| this.sourceKey = options.sourceKey ?? null; | ||
| this.sourceKey = options.sourceKey instanceof Key ? options.sourceKey.name : options.sourceKey ?? null; |
There was a problem hiding this comment.
[BestPractice]
The logic to resolve the sourceKey from either a string or a Key instance is duplicated in the constructors for VectorIndexConfig (line 75) and SparseVectorIndexConfig (line 95).
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you could extract this logic into a shared helper function.
For example:
function resolveSourceKey(sourceKey: string | Key | null | undefined): string | null {
return sourceKey instanceof Key ? sourceKey.name : sourceKey ?? null;
}Then, you can simplify both constructors:
// In VectorIndexConfig constructor
this.sourceKey = resolveSourceKey(options.sourceKey);
// In SparseVectorIndexConfig constructor
this.sourceKey = resolveSourceKey(options.sourceKey);Context for Agents
[**BestPractice**]
The logic to resolve the `sourceKey` from either a `string` or a `Key` instance is duplicated in the constructors for `VectorIndexConfig` (line 75) and `SparseVectorIndexConfig` (line 95).
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you could extract this logic into a shared helper function.
For example:
```typescript
function resolveSourceKey(sourceKey: string | Key | null | undefined): string | null {
return sourceKey instanceof Key ? sourceKey.name : sourceKey ?? null;
}
```
Then, you can simplify both constructors:
```typescript
// In VectorIndexConfig constructor
this.sourceKey = resolveSourceKey(options.sourceKey);
// In SparseVectorIndexConfig constructor
this.sourceKey = resolveSourceKey(options.sourceKey);
```
File: clients/new-js/packages/chromadb/src/schema.ts
Line: 95| it("accepts Key instance for VectorIndexConfig sourceKey", () => { | ||
| const { K } = require("../src/execution"); | ||
| const schema = new Schema(); | ||
| const vectorConfig = new VectorIndexConfig({ | ||
| sourceKey: K.DOCUMENT, | ||
| }); | ||
|
|
||
| expect(vectorConfig.sourceKey).toBe("#document"); | ||
|
|
||
| // Also test with custom key | ||
| const customKey = K("myfield"); | ||
| const vectorConfig2 = new VectorIndexConfig({ | ||
| sourceKey: customKey, | ||
| }); | ||
|
|
||
| expect(vectorConfig2.sourceKey).toBe("myfield"); | ||
| }); | ||
|
|
||
| it("accepts Key instance for SparseVectorIndexConfig sourceKey", () => { |
There was a problem hiding this comment.
[BestPractice]
The tests accepts Key instance for VectorIndexConfig sourceKey and accepts Key instance for SparseVectorIndexConfig sourceKey are nearly identical. You can use test.each to avoid this duplication and make the tests more concise.
The schema variable is also unused in these tests and can be removed.
Here's an example of how you could refactor this:
describe.each([
{ ConfigClass: VectorIndexConfig, name: "VectorIndexConfig" },
{ ConfigClass: SparseVectorIndexConfig, name: "SparseVectorIndexConfig" },
])("for $name", ({ ConfigClass }) => {
it("accepts Key instance for sourceKey", () => {
const { K } = require("../src/execution");
const config = new ConfigClass({
sourceKey: K.DOCUMENT,
});
expect(config.sourceKey).toBe("#document");
const customKey = K("myfield");
const config2 = new ConfigClass({
sourceKey: customKey,
});
expect(config2.sourceKey).toBe("myfield");
});
});This single test block could replace both of the individual tests from lines 1713-1747.
Context for Agents
[**BestPractice**]
The tests `accepts Key instance for VectorIndexConfig sourceKey` and `accepts Key instance for SparseVectorIndexConfig sourceKey` are nearly identical. You can use `test.each` to avoid this duplication and make the tests more concise.
The `schema` variable is also unused in these tests and can be removed.
Here's an example of how you could refactor this:
```typescript
describe.each([
{ ConfigClass: VectorIndexConfig, name: "VectorIndexConfig" },
{ ConfigClass: SparseVectorIndexConfig, name: "SparseVectorIndexConfig" },
])("for $name", ({ ConfigClass }) => {
it("accepts Key instance for sourceKey", () => {
const { K } = require("../src/execution");
const config = new ConfigClass({
sourceKey: K.DOCUMENT,
});
expect(config.sourceKey).toBe("#document");
const customKey = K("myfield");
const config2 = new ConfigClass({
sourceKey: customKey,
});
expect(config2.sourceKey).toBe("myfield");
});
});
```
This single test block could replace both of the individual tests from lines 1713-1747.
File: clients/new-js/packages/chromadb/test/schema.test.ts
Line: 1731
Sicheng-Pan
force-pushed
the
11-06-_doc_fix_mistakes_in_examples
branch
from
1a8e5bb to
70b442b
Compare
Sicheng-Pan
force-pushed
the
11-06-_bug_ts_vector_config_source_key_should_accept_key
branch
from
ed794bd to
c0291c1
Compare
This comment has been minimized.
This comment has been minimized.
Merge activity
|
Sicheng-Pan
changed the base branch from
11-06-_doc_fix_mistakes_in_examples
to
graphite-base/5840
Sicheng-Pan
deleted the
11-06-_bug_ts_vector_config_source_key_should_accept_key
branch
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?