Skip to content

fix(server): skip unsafe count optm for nested predicates#3100

Merged
imbajin merged 4 commits into
apache:masterfrom
lokidundun:fix-issue-3085
Jul 23, 2026
Merged

fix(server): skip unsafe count optm for nested predicates#3100
imbajin merged 4 commits into
apache:masterfrom
lokidundun:fix-issue-3085

Conversation

@lokidundun

Copy link
Copy Markdown
Contributor

Purpose of the PR

Main Changes

  • Avoid applying HugeCountStrategy to count().is(...) traversals
    containing nested connective predicates.
  • Keep the original CountGlobalStep + IsStep traversal for nested
    predicates, so that TinkerPop evaluates the complete predicate tree.
  • Preserve the existing optimization for simple predicates and flat
    connective predicates.
  • Prevent partially analyzed nested predicates from being transformed
    into RangeGlobalStep or NotStep, which can change or invert the
    filtering semantics.
  • Add regression tests for a nested connective predicate and its
    negated form.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working tests Add or improve test cases labels Jul 20, 2026

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: Flat connective count predicates remain semantically unsafe and can return incorrect results after the destructive NotStep rewrite. Evidence: exact-head control-flow review and two independent TinkerPop 3.5.1 reproductions.

}

final P<?> predicate = ((IsStep<?>) step.getNextStep()).getPredicate();
if (this.hasNestedConnectivePredicate(predicate)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

‼️ Critical: flat connective predicates remain semantically unsafe

This guard only rejects a ConnectiveP whose child is another ConnectiveP, so flat expressions such as P.eq(0L).and(P.gt(0L)) and P.lte(0L).or(P.gte(1L)) still enter the optimization. The loop then derives useNotStep from only the first range-driving child and removes the complete count().is(...); on an empty adjacency the contradiction can return true, and on a non-empty adjacency the tautology can return false. Both shapes were reproduced against the current strategy on TinkerPop 3.5.1. Please disable the destructive rewrite for every ConnectiveP unless equivalence is derived from the complete boolean expression; the conservative fix is to skip all ConnectiveP values here. Add flat AND/OR regressions for both zero and nonzero counts.

@lokidundun

Copy link
Copy Markdown
Contributor Author

This ci failure is not from this PR.HugeCountStrategy only fires on Gremlin traversals with count().is(...), but KoutTraverser uses HugeGraph's own BFS framework (no Gremlin traversal at all). The change only skips more cases,.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: yes. Summary: The blanket ConnectiveP guard fixes the incorrect-result cases, but it also removes safe range limiting for common flat count predicates and can turn bounded counts into full scans. Evidence: exact-head control-flow inspection, confirmed independently by two review lanes.

}

final P<?> predicate = ((IsStep<?>) step.getNextStep()).getPredicate();
if (predicate instanceof ConnectiveP) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

⚠️ Important: preserve safe range limiting for flat connective predicates

Returning here for every ConnectiveP bypasses the entire strategy, including the safe RangeGlobalStep insertion. Common predicates such as between(1, 18) are connective predicates, so a high-degree count().is(...) traversal now counts the complete input instead of stopping at the upper threshold. The new result assertions do not detect this plan regression. Please keep disabling the destructive NotStep/dismissCountIs rewrites unless full-expression equivalence is proven, while retaining a proven-safe range cap for analyzable flat connectives (and continue skipping nested cases as needed). Add a traversal-plan regression that asserts a representative flat connective still receives the range bound.

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 22, 2026

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: no. Summary: The current head preserves safe range limiting for flat connective predicates while bypassing unsafe nested predicate rewrites; no actionable issues were found. Evidence: CountStrategyCoreTest passed 35/35 on the exact head, and all 15 visible latest-head checks succeeded.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 23, 2026
@imbajin imbajin changed the title fix(core): skip unsafe count optimization for nested predicates fix(server): skip unsafe count optm for nested predicates Jul 23, 2026
@imbajin
imbajin merged commit e960cc5 into apache:master Jul 23, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files. tests Add or improve test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Wrong result for where(count().is()

2 participants