-
-
Notifications
You must be signed in to change notification settings - Fork 79
Add support for join relations in facet and numeric filters #260
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
base: master
Are you sure you want to change the base?
Conversation
* Initial plan * Initial plan for adding join relations support Co-authored-by: OllieJennings <[email protected]> * Add join relations support to facet and numeric filters Co-authored-by: OllieJennings <[email protected]> * Remove temporary jest config and update gitignore Co-authored-by: OllieJennings <[email protected]> * Revert formatting change in FacetSearchResponseAdapter.js Co-authored-by: OllieJennings <[email protected]> * Remove jest.unit.config.js from .gitignore Co-authored-by: OllieJennings <[email protected]> * Refactor to eliminate code duplication in join relation handling Co-authored-by: OllieJennings <[email protected]> * Fix FacetSearchResponseAdapter quotes and eliminate duplicate if statement Co-authored-by: OllieJennings <[email protected]> * Revert FacetSearchResponseAdapter to single quotes Co-authored-by: OllieJennings <[email protected]> * Build lib and dist files with join relations support Co-authored-by: OllieJennings <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: OllieJennings <[email protected]>
There was a problem hiding this comment.
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 adds support for join relations in facet and numeric filters to prepare the adapter for Typesense v30's new join relations feature. The implementation enables filtering on related data using a special syntax like $product_prices(field).
- Introduces regex pattern matching for joined relation filter syntax
- Adds a helper method to build filter strings for both regular and joined relation filters
- Updates numeric and facet filter adapters to handle joined relation syntax
Reviewed Changes
Copilot reviewed 3 out of 8 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/SearchRequestAdpater.test.js | Adds comprehensive test coverage for joined relation filters in both numeric and facet filter scenarios |
| src/SearchRequestAdapter.js | Implements core logic for parsing and adapting joined relation filters with new regex pattern and helper method |
| lib/SearchRequestAdapter.js | Generated/compiled version of the source changes with updated variable scoping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Could you also add some integration tests under the testground folder? |
Yeah of course |
* Initial plan * Add integration tests for numeric refinement list, pagination/sorting, and range filters Co-authored-by: OllieJennings <[email protected]> * Fix linting issues in integration tests Co-authored-by: OllieJennings <[email protected]> * Add documentation for new integration tests Co-authored-by: OllieJennings <[email protected]> * Add comprehensive PR summary documentation Co-authored-by: OllieJennings <[email protected]> * Remove incorrectly placed test files and revert FacetSearchResponseAdapter Co-authored-by: OllieJennings <[email protected]> * Add integration tests for joins functionality in testground Co-authored-by: OllieJennings <[email protected]> * Rename products_with_prices collection to products Co-authored-by: OllieJennings <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: OllieJennings <[email protected]>
|
@tharropoulos Updated |
…e filter handling (#3)
…perator (#4) * Initial plan * Initial plan: Refactor ternary operator to check isExcluded first Co-authored-by: OllieJennings <[email protected]> * Refactor: Check isExcluded first in _buildFacetFilterString ternary operator Co-authored-by: OllieJennings <[email protected]> * Build: Update dist files after refactoring Co-authored-by: OllieJennings <[email protected]> * Revert: Undo quote style change in FacetSearchResponseAdapter Co-authored-by: OllieJennings <[email protected]> * Add DOM tests for joins page Co-authored-by: OllieJennings <[email protected]> * Revert: Fix quote style in src/FacetSearchResponseAdapter.js Co-authored-by: OllieJennings <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: OllieJennings <[email protected]>
* fix: configure babel to preserve esm modules instead of converting to cjs • set modules: false in @babel/preset-env to maintain esm syntax • prevents commonjs interop issues when importing from esm contexts • resolves publint warning about __esModule and exports.default pattern * fix(test): let jest still interpret files as cjs --------- Co-authored-by: Fanis Tharropoulos <[email protected]> Co-authored-by: Jason Bosco <[email protected]>
|
@tharropoulos Can you look again? Can confirm this was tested and all running fine against my local, and QA branch |
|
@jasonbosco lgtm |
* Initial plan * Initial exploration and planning Co-authored-by: OllieJennings <[email protected]> * Implement join filter grouping by collection Co-authored-by: OllieJennings <[email protected]> * Revert lib/Configuration.js and lib/support/utils.js to use CommonJS format Co-authored-by: OllieJennings <[email protected]> * Move join filter grouping to _adaptFilters for cross-filter grouping Co-authored-by: OllieJennings <[email protected]> * Revert quote style change in src/FacetSearchResponseAdapter.js Co-authored-by: OllieJennings <[email protected]> * Fix test expectations for _adaptFacetFilters to expect ungrouped output Co-authored-by: OllieJennings <[email protected]> * Regenerate all lib files from source Co-authored-by: OllieJennings <[email protected]> * Fully regenerate lib files with ESM format Co-authored-by: OllieJennings <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: OllieJennings <[email protected]>
|
|
||
| filters.forEach((filter) => { | ||
| // Match pattern like "$collection(field:=value)" or "$collection(field:>=value)" | ||
| const joinMatch = filter.match(/^(\$[^(]+)\((.*)\)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex syntax is starting to get super complicated. It should have a dedicated test file for it. I'd advise starting to just parse things out manually, token by token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep i feel you, its getting a bit complex, let me do that
Change Summary
Add support for join relations in the facet filters (standard & numeric).
Join relations are a new feature being introduced by typesense v30, this PR gets the adapter ready to support this new feature.
PR Checklist