Skip to content

feat(query): add shared query base classes#25

Open
nkanu17 wants to merge 5 commits into
mainfrom
stack/query-core-on-hybrid
Open

feat(query): add shared query base classes#25
nkanu17 wants to merge 5 commits into
mainfrom
stack/query-core-on-hybrid

Conversation

@nkanu17

@nkanu17 nkanu17 commented May 13, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds the shared query foundation needed for the remaining RedisVL Python parity work.

  • Adds BaseQuery for common query state, validation, return fields, paging, and query-level sorting.
  • Adds BaseVectorQuery for vector-specific state, datatype normalization, defensive vector copying, and shared vector validation.
  • Moves VectorQuery, VectorRangeQuery, FilterQuery, CountQuery, TextQuery, HybridQuery, and AggregationQuery onto the shared base classes without changing their constructor-facing APIs.
  • Adds chainable query helpers:
    • setFilter(...)
    • setReturnFields(...)
    • paging(...)
    • sortBy(...)
  • Wires query-level sortBy() through SearchIndex.search() while preserving options.sortBy precedence.

Release note

  • Breaking: exported BaseQuery is now an abstract class instead of an interface; external implementers need to extend it or adapt to concrete query classes.

Rebase

Rebased onto current main after the dependent query branches landed.

TDD Notes

Tests were written first and failed before implementation:

  • tests/unit/query/base.test.ts
    • BaseQuery common config, filter updates, return fields, skip-decode fields, paging, sorting, and validation.
    • BaseVectorQuery vector state, datatype normalization, defensive copying, and validation.
  • tests/unit/query/vector.test.ts
    • VectorQuery inheritance from BaseVectorQuery / BaseQuery.
    • Chainable paging, setReturnFields, and setFilter behavior.
  • tests/unit/query/hybrid.test.ts
    • HybridQuery inheritance from BaseVectorQuery / BaseQuery.
  • tests/unit/query/aggregation.test.ts
    • AggregationQuery inheritance from BaseQuery and inherited return-field loading.
  • tests/unit/indexes/search-index.test.ts
    • Query-level sortBy() reaches FT.SEARCH options.
    • options.sortBy overrides query-level sorting.

Implementation followed after the failing tests were in place.

Verification

  • npm run type-check
  • npm run type-check:tests
  • npm run test:unit
  • npx 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

@nkanu17

nkanu17 commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

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.

@nkanu17 nkanu17 closed this May 13, 2026
@nkanu17 nkanu17 reopened this May 13, 2026
@nkanu17

nkanu17 commented May 13, 2026

Copy link
Copy Markdown
Contributor Author

PR Notes

Reopened this as the next feature branch stacked on #9.

  • Base branch: feat/hybrid-search
  • Head branch: stack/query-core-on-hybrid
  • Scope: shared query foundation only

This should stay blocked on #9 landing first.

Base automatically changed from feat/hybrid-search to main May 22, 2026 20:11
@banker

banker commented May 22, 2026

Copy link
Copy Markdown
Contributor

Thanks @nkanu17 — the BaseQuery / BaseVectorQuery design and the chainable helpers look great, and the test coverage is solid. A few items before merge:

1. Rebase onto current main

mergeable: CONFLICTINGfeat/hybrid-search (#9), feat/aggregation-query-v2 (#28), and feat/redis-client-resp2-guard (#29) have all landed on main since this branch opened, so a rebase is needed.

2. Migrate HybridQuery and AggregationQuery onto BaseQuery in this PR

Both classes landed on main after this PR was opened, and neither extends BaseQuery:

  • src/query/hybrid.ts:245export class HybridQuery {
  • src/query/aggregation.ts:227export class AggregationQuery {

If they don't get migrated here, we ship main with split inheritance (5 query types on BaseQuery, 2 not) and a follow-up is guaranteed. I'd much rather they ride along with this PR than leave a known cleanup behind — please fold them in.

AggregationQuery is the easier of the two (FT.AGGREGATE doesn't share all of FT.SEARCH's state, so it may only inherit the filter/returnFields surface — feel free to push back if it doesn't fit cleanly). HybridQuery should be a natural fit since FT.HYBRID is closer to FT.SEARCH semantics.

3. Document sortBy precedence

SearchIndex.search() now sets SORTBY from query.sortFields[0], then overwrites it from options.sortBy if both are supplied. That's a reasonable choice (preserves the existing search() signature), but it needs to be documented on the SearchIndex.search JSDoc so callers know which wins. Worth a comment on the searchOptions.SORTBY assignment too.

4. Note the interface→class change in the PR body

Converting BaseQuery from an exported interface to an exported abstract class is technically a breaking change for anyone implementing the interface externally. At v0.1.0-beta this is fine, but please add a one-line note in the PR description so it ends up in the release notes when we cut the next version.

Re-review once #2 is in. Heads up that #31 (TextQuery stopwords) will likely land first — I've told that author they won't need to rebase against this PR.

@nkanu17 nkanu17 force-pushed the stack/query-core-on-hybrid branch from d28381d to 56e94c5 Compare June 4, 2026 03:22
Copilot AI review requested due to automatic review settings June 4, 2026 03:22
@nkanu17

nkanu17 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@banker pushed the requested updates.

Changes made:

  1. Rebased PR 25 onto current main.
  2. Moved HybridQuery onto BaseVectorQuery and AggregationQuery onto BaseQuery.
  3. Kept constructor facing APIs intact and preserved HybridQuery config sort handling.
  4. Documented SearchIndex.search sort precedence and kept options.sortBy as the winner over query sortBy.
  5. Added the BaseQuery interface to abstract class release note to the PR body.
  6. Added tests for HybridQuery and AggregationQuery inheritance, query sort wiring, and options sort precedence.

Verification:

  1. npm run type-check
  2. npm run type-check:tests
  3. npm run test:unit

Commit: 56e94c5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 / BaseVectorQuery foundations (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.

Comment thread src/query/base.ts
Comment thread src/query/base.ts
Comment thread src/query/base.ts
Comment thread src/query/count.ts
Copilot AI review requested due to automatic review settings June 4, 2026 03:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.sortBy is provided without options.sortOrder, this code assigns searchOptions.SORTBY to a bare string. The @redis/search FT.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,

Comment thread src/query/base.ts
Comment thread src/query/base.ts
Comment thread src/query/base.ts Outdated
Comment thread src/query/hybrid.ts
Copilot AI review requested due to automatic review settings June 4, 2026 04:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

@nkanu17 nkanu17 requested review from banker and booleanhunter June 4, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants