feat(query): add shared query base classes#25
Conversation
|
Closing this for now. This stacked Query Core branch should only become a PR if/when we explicitly decide to add new scope on top of #9. |
|
Thanks @nkanu17 — the 1. Rebase onto current
|
d28381d to
56e94c5
Compare
|
@banker pushed the requested updates. Changes made:
Verification:
Commit: 56e94c5 |
There was a problem hiding this comment.
Pull request overview
This PR introduces shared, chainable base classes for query building (BaseQuery and BaseVectorQuery) to consolidate common query state/validation (filters, return fields, paging, and query-level sorting) and migrates existing query types to inherit from them. It also wires query-level sortBy() into SearchIndex.search() while preserving execution-option sort precedence.
Changes:
- Added
BaseQuery/BaseVectorQueryfoundations (filter/return-fields/paging/sort state + validation, plus vector datatype normalization and defensive vector copying). - Updated existing query classes (e.g.,
VectorQuery,TextQuery,FilterQuery,CountQuery,VectorRangeQuery,HybridQuery,AggregationQuery) to extend the shared base classes without changing constructor-facing APIs. - Extended
SearchIndex.search()to apply query-level sorting and updated/added unit tests validating inheritance and chaining behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/query/base.test.ts | New unit coverage for BaseQuery / BaseVectorQuery shared behavior and validation. |
| tests/unit/query/vector.test.ts | Verifies VectorQuery inherits base classes and supports new chainable helpers. |
| tests/unit/query/text.test.ts | Asserts TextQuery now extends BaseQuery. |
| tests/unit/query/range.test.ts | Asserts VectorRangeQuery now extends vector + base query classes. |
| tests/unit/query/hybrid.test.ts | Asserts HybridQuery now extends vector + base query classes. |
| tests/unit/query/filter-query.test.ts | Asserts FilterQuery now extends BaseQuery. |
| tests/unit/query/count.test.ts | Asserts CountQuery now extends BaseQuery and retains count-query semantics. |
| tests/unit/query/aggregation.test.ts | Asserts AggregationQuery now extends BaseQuery and maps inherited return fields to LOAD. |
| tests/unit/indexes/search-index.test.ts | Adds coverage for query-level sortBy() reaching FT.SEARCH, with options.sortBy precedence. |
| src/query/base.ts | Implements BaseQuery / BaseVectorQuery, shared config/types, chaining, and validation. |
| src/query/vector.ts | Migrates VectorQuery to extend BaseVectorQuery and delegate shared behavior to base classes. |
| src/query/text.ts | Migrates TextQuery to extend BaseQuery and delegate shared behavior to base classes. |
| src/query/range.ts | Migrates VectorRangeQuery to extend BaseVectorQuery and delegate shared behavior to base classes. |
| src/query/hybrid.ts | Migrates HybridQuery to extend BaseVectorQuery and composes hybrid-specific sort with base sortBy(). |
| src/query/filter-query.ts | Migrates FilterQuery to extend BaseQuery and delegate shared behavior to base classes. |
| src/query/count.ts | Migrates CountQuery to extend BaseQuery and rely on shared filter handling. |
| src/query/aggregation.ts | Migrates AggregationQuery to extend BaseQuery and incorporate inherited return-fields into LOAD. |
| src/indexes/search-index.ts | Applies query-level sort fields and switches pagination reads to getOffset/getLimit. |
| src/index.ts | Exports the new base classes and shared query types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/indexes/search-index.ts:629
- When
options.sortByis provided withoutoptions.sortOrder, this code assignssearchOptions.SORTBYto a bare string. The@redis/searchFT.SEARCH option expects an object like{ BY, DIRECTION? }, so this can send an invalid options shape. Always build the object form (and include DIRECTION only when provided).
if (options?.sortBy) {
searchOptions.SORTBY = options.sortBy;
if (options.sortOrder) {
searchOptions.SORTBY = {
BY: options.sortBy,
Summary
This PR adds the shared query foundation needed for the remaining RedisVL Python parity work.
BaseQueryfor common query state, validation, return fields, paging, and query-level sorting.BaseVectorQueryfor vector-specific state, datatype normalization, defensive vector copying, and shared vector validation.VectorQuery,VectorRangeQuery,FilterQuery,CountQuery,TextQuery,HybridQuery, andAggregationQueryonto the shared base classes without changing their constructor-facing APIs.setFilter(...)setReturnFields(...)paging(...)sortBy(...)sortBy()throughSearchIndex.search()while preservingoptions.sortByprecedence.Release note
BaseQueryis now an abstract class instead of an interface; external implementers need to extend it or adapt to concrete query classes.Rebase
Rebased onto current
mainafter the dependent query branches landed.TDD Notes
Tests were written first and failed before implementation:
tests/unit/query/base.test.tsBaseQuerycommon config, filter updates, return fields, skip-decode fields, paging, sorting, and validation.BaseVectorQueryvector state, datatype normalization, defensive copying, and validation.tests/unit/query/vector.test.tsVectorQueryinheritance fromBaseVectorQuery/BaseQuery.paging,setReturnFields, andsetFilterbehavior.tests/unit/query/hybrid.test.tsHybridQueryinheritance fromBaseVectorQuery/BaseQuery.tests/unit/query/aggregation.test.tsAggregationQueryinheritance fromBaseQueryand inherited return-field loading.tests/unit/indexes/search-index.test.tssortBy()reachesFT.SEARCHoptions.options.sortByoverrides query-level sorting.Implementation followed after the failing tests were in place.
Verification
npm run type-checknpm run type-check:testsnpm run test:unitnpx vitest run --config vitest.unit.config.ts tests/unit/query/base.test.ts tests/unit/query/vector.test.ts tests/unit/query/count.test.ts tests/unit/query/filter-query.test.ts tests/unit/query/text.test.ts tests/unit/query/range.test.ts tests/unit/query/hybrid.test.ts tests/unit/query/aggregation.test.ts tests/unit/indexes/search-index.test.ts