fix: add support for LSM sparse vector type in BoltNetworkExecutor#4079
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | Report missing for 95c57ba1 |
| Diff coverage | ✅ 66.67% diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (95c57ba) Report Missing Report Missing Report Missing Head commit (0c92cc4) 152973 100173 65.48% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4079) 6 4 66.67% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Code ReviewOverviewThis PR adds Bug: Routing condition is incompleteFile: The } else if (normalized.startsWith("show index")
|| normalized.startsWith("show all index")
|| normalized.startsWith("show range index")
|| normalized.startsWith("show text index")
|| normalized.startsWith("show point index")
|| normalized.startsWith("show lookup index")
|| normalized.startsWith("show fulltext index")
|| normalized.startsWith("show vector index")) {
Fix: } else if (normalized.startsWith("show index")
|| normalized.startsWith("show all index")
|| normalized.startsWith("show range index")
|| normalized.startsWith("show text index")
|| normalized.startsWith("show point index")
|| normalized.startsWith("show lookup index")
|| normalized.startsWith("show fulltext index")
|| normalized.startsWith("show vector index")
|| normalized.startsWith("show sparse_vector index")) {Missing test coverageThe PR checklist says unit tests were written, but the diff contains no new test code. There is already a pattern to follow in @Test
void showVectorIndexesReturnsEmptyWithoutVectorIndexes() { ... }A parallel test for @Test
void showSparseVectorIndexesReturnsEmptyWithoutSparseVectorIndexes() {
try (Driver driver = getDriver()) {
try (Session session = driver.session(SessionConfig.forDatabase(getDatabaseName()))) {
final Result result = session.run("SHOW SPARSE_VECTOR INDEXES");
assertThat(result.keys()).containsExactly("id", "name", "state", "populationPercent", "type",
"entityType", "labelsOrTypes", "properties", "indexProvider", "owningConstraint", "lastRead", "readCount");
final List<Record> rows = result.list();
for (final Record r : rows)
assertThat(r.get("type").asString()).isEqualTo("SPARSE_VECTOR");
}
}
}Minor: Provider string convention
Summary
The routing omission is a real functional gap - please add |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the LSM_SPARSE_VECTOR index type within the BoltNetworkExecutor, including its mapping to Neo4j-compatible strings and provider versions. While the filtering logic for system queries was updated, feedback indicates that the handleSystemQuery method must also be modified to intercept the new SHOW SPARSE_VECTOR INDEXES command, otherwise the functionality remains unreachable.
| if (filter.startsWith("show sparse_vector index")) | ||
| return "SPARSE_VECTOR".equals(neoType); |
There was a problem hiding this comment.
The addition of LSM_SPARSE_VECTOR support in indexTypeMatchesFilter is correct, but for this to be reachable, the handleSystemQuery method (around line 1079) also needs to be updated to intercept the SHOW SPARSE_VECTOR INDEXES command. Currently, it only intercepts a specific set of 'show' commands, and sparse_vector is missing from that list. Without this interception, the command will not be recognized as a system query and will likely fail when passed to the query engine.
Code Review
OverviewThis PR adds
A regression test is included. CorrectnessThe implementation is correct and complete. The exhaustive Test CoverageThe new test Suggestion: Consider adding a positive-path test that actually creates a sparse vector index and confirms it appears in Minor Observations
SummaryThe fix is correct and well-structured. The only real gap is the absence of a positive test case that exercises the filter path with an actual sparse vector index. Everything else LGTM. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4079 +/- ##
==========================================
+ Coverage 64.36% 64.40% +0.03%
==========================================
Files 1598 1603 +5
Lines 120818 121624 +806
Branches 25743 25940 +197
==========================================
+ Hits 77766 78327 +561
- Misses 32294 32452 +158
- Partials 10758 10845 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
What does this PR do?
fix bolt executor adding map for sparse vector
Checklist
mvn clean packagecommand