fix(server): avoid extracting text range filters#3034
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a traversal query-planning bug where range predicates (gt/gte/lt/lte) on Text properties were being extracted/pushed down into backend queries, leading to incorrect behavior (and the exception reported in #2935). The change keeps these text range predicates in the traversal layer while adding a regression test to ensure direct traversals behave consistently with equivalent match() traversals.
Changes:
- Prevent extraction of
HasStepcontainers when they include range comparisons onTextproperties. - Keep existing extraction behavior for system properties and non-text property predicates (intended).
- Add a regression test reproducing the traversal shape from #2935 and asserting consistent results between direct and
match()forms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java | Adds a guard to stop extracting HasContainers with text range predicates into backend query steps. |
| hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/core/CountStrategyCoreTest.java | Adds a regression test covering a text range filter followed by repeat(...).emit().times(1).count(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fix/fix-text-range-filter # Conflicts: # hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3034 +/- ##
=============================================
+ Coverage 35.94% 93.25% +57.31%
+ Complexity 338 65 -273
=============================================
Files 803 9 -794
Lines 68053 267 -67786
Branches 8907 22 -8885
=============================================
- Hits 24460 249 -24211
+ Misses 40968 8 -40960
+ Partials 2625 10 -2615 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
imbajin
left a comment
There was a problem hiding this comment.
The original correctness fix is clear, but the latest version now broadens the PR into a more aggressive HasStep split-and-remove optimization. I left one scope/risk comment suggesting we keep this PR on the minimal safe path and move the broader optimization to a follow-up.
imbajin
left a comment
There was a problem hiding this comment.
The current patch looks scoped back to the safer minimal fix. I left three non-blocking test-coverage suggestions to better guard the text-range extraction behavior and related fallback paths.
Purpose of the PR
This PR fixes the query-planning path exposed by #2935.
For top-level traversals like
g.V().has("vp4", P.lt("")).repeat(...).count(),HugeGraph may extract the text range predicate into a backend query. However,
range predicates on text properties should not be planned as backend range
index queries. This can make the direct traversal fail while an equivalent
match()traversal returns normally.This change keeps such text range predicates in the traversal layer instead of
pushing them down to the backend query, making the direct traversal consistent
with the equivalent
match()form.Main Changes
Textproperty predicates withgt/gte/lt/lteintoHugeGraphStep/HugeVertexStepbackend queries.predicates.
has("vp4", P.lt("")).repeat(__.out("el2")).emit().times(1).count().match()traversal.Verifying these changes
-
mvn -pl hugegraph-server/hugegraph-test -am -P core-test,memory -Dtest=CountStrategyCoreTest#testRepeatAfterTextRangeFilterWithEmptyResult -DfailIfNoTests=false "-Drat.skip=true" "-Dcheckstyle.skip=true" testmvn -pl hugegraph-server/hugegraph-test -am -P core-test,rocksdb -Dtest=CountStrategyCoreTest#testRepeatAfterTextRangeFilterWithEmptyResult -DfailIfNoTests=false "-Drat.skip=true" "-Dcheckstyle.skip=true" testDoes this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need