Skip to content

fix(server): clarify condition resolution semantics for label queries - #2994

Open
contrueCT wants to merge 27 commits into
apache:masterfrom
contrueCT:task/improve-condition-query-semantics
Open

fix(server): clarify condition resolution semantics for label queries#2994
contrueCT wants to merge 27 commits into
apache:masterfrom
contrueCT:task/improve-condition-query-semantics

Conversation

@contrueCT

Copy link
Copy Markdown
Contributor

Purpose of the PR

ConditionQuery.condition() currently mixes several different meanings in one API, including:

  • no condition
  • conflicting conditions resolved to empty
  • a unique resolved value
  • a raw multi-value result
  • an exception for ambiguous resolved values

This PR keeps the legacy condition() behavior unchanged, adds explicit condition-resolution APIs, and migrates the high-risk LABEL call sites to use the clearer semantics.

Main Changes

  • Add explicit condition-resolution APIs to ConditionQuery
    • containsCondition(Object key)
    • conditionValues(Object key)
    • conditionValue(Object key)
  • Keep the legacy condition() method backward-compatible
  • Document the semantic differences between the legacy API and the new explicit APIs
  • Migrate LABEL-related high-risk callers to the new APIs in:
    • graph/index transactions
    • serializers
    • traversers
    • in-memory / hstore paths
  • Preserve the old behavior for non-LABEL legacy usages in this first step

Verifying these changes

Added and extended regression coverage for the new semantics:

  • QueryTest#testConditionWithoutLabel
  • QueryTest#testConditionWithEqAndIn
  • QueryTest#testConditionWithSingleInValues
  • QueryTest#testConditionWithConflictingEqAndIn
  • QueryTest#testConditionWithMultipleMatchedInValues

Added a targeted regression for the label-index fallback path:

  • VertexCoreTest#testCollectMatchedIndexesByJointLabelsWithIndexedProperties

This test verifies:

  • a multi-label query can conservatively fall back and still match the indexed label
  • conflicting label conditions produce no matched indexes

Existing label-query regressions were also rechecked to ensure no behavior regression:

  • EdgeCoreTest#testQueryInEdgesOfVertexByLabels
  • EdgeCoreTest#testQueryInEdgesOfVertexByConflictingLabels
  • EdgeCoreTest#testQueryInEdgesOfVertexBySortkey
  • VertexCoreTest#testQueryByJointLabels
  • 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:
    • mvn -pl hugegraph-server/hugegraph-test -am -P core-test,memory -DfailIfNoTests=false -Dtest='QueryTest#testConditionWithoutLabel+testConditionWithEqAndIn+testConditionWithSingleInValues+testConditionWithConflictingEqAndIn+testConditionWithMultipleMatchedInValues' test
    • mvn -pl hugegraph-server/hugegraph-test -am -P core-test,memory -DfailIfNoTests=false -Dtest='EdgeCoreTest#testQueryInEdgesOfVertexByLabels+testQueryInEdgesOfVertexByConflictingLabels+testQueryInEdgesOfVertexBySortkey' test
    • mvn -pl hugegraph-server/hugegraph-test -am -P core-test,memory -DfailIfNoTests=false -Dtest='VertexCoreTest#testQueryByJointLabels+testCollectMatchedIndexesByJointLabelsWithIndexedProperties' test

Does this PR potentially affect the following parts?

Documentation Status

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

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 13, 2026
@contrueCT contrueCT changed the title improve(query): clarify condition resolution semantics for label queries fix(query): clarify condition resolution semantics for label queries Apr 19, 2026
@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.25%. Comparing base (e960cc5) to head (61e5bbc).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master    #2994       +/-   ##
=============================================
+ Coverage     42.32%   93.25%   +50.93%     
+ Complexity      460       65      -395     
=============================================
  Files           819        9      -810     
  Lines         70745      267    -70478     
  Branches       9366       22     -9344     
=============================================
- Hits          29944      249    -29695     
+ Misses        37705        8    -37697     
+ Partials       3096       10     -3086     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch 2 times, most recently from 4c42786 to cc9af24 Compare May 26, 2026 12:29

@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.

I found one correctness issue in the latest revision. The CI failures were posted separately as a PR-level reminder.

CI/status checks are failing on the latest head (cc9af24929e42af1c90e1f55f3e60adc351e0318). Could you check the failed jobs before the next review round?

Failed checks include:

@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from cc9af24 to 2e82f83 Compare May 30, 2026 10:20

@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.

I don't see a clear blocking correctness issue in the latest head, and the previous LABEL-resolution comments look addressed. One remaining merge risk is that the latest checks are still red: hstore failed in VertexCoreTest#testQueryByDateProperty.

Since this PR also touches HstoreStore, could you rerun or clarify whether the hstore failure is an existing flaky/environment issue?

contrueCT added 5 commits June 5, 2026 12:59
Add explicit condition resolution APIs to ConditionQuery while preserving the legacy condition() behavior. Introduce containsCondition(Object), conditionValues(Object), and conditionValue(Object) so callers can distinguish missing, empty, unique, and multi-value results without overloading null semantics.

Migrate LABEL-specific consumers in graph/index transactions, serializers, traversers, and stores to use the new APIs for unique-label resolution and conservative fallback behavior. Extend QueryTest and VertexCoreTest to cover absent, conflicting, and multi-value label conditions as well as collectMatchedIndexes() behavior for multi-label and conflicting label queries.
@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from 94408b7 to b10e3c2 Compare June 5, 2026 05:10
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 5, 2026
@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from 801923a to ebc31c8 Compare June 5, 2026 18:06
@contrueCT

contrueCT commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for your patience. The hstore CI failure exposed an existing latent issue in hstore's range-index query path. For range-index scans with limit/paging, the upper layer assumed that backend scan results were globally ordered by the range-index key and that the returned page state could be reused as a HugeGraph range cursor. In hstore, multi-node/tablet scans can return entries in backend iterator order, and the page state is an internal storage cursor, so those assumptions may lead to unstable ordering or skipped results. This PR keeps the fix intentionally scoped: hstore range-index queries whose visible result depends on limit/offset/paging are sorted and sliced in the index layer, while unbounded scans still use the original streaming path to avoid disturbing count, joint-index, and cleanup paths. I think this is enough for the current PR, but the underlying hstore scan/page-state contract should be handled in a dedicated follow-up, ideally by defining whether range scans must be globally ordered and fixing the hstore iterator/page-state semantics at the storage-client layer.

@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: HStore range-index offset queries can skip too many sorted results. Evidence: static review of GraphIndexTransaction/query offset handling.

@contrueCT

Copy link
Copy Markdown
Contributor Author

Thanks. I fixed this by resetting scanQuery.offset(0L) before the full sorted range-index scan, so the fallback now reads the complete matched range first and lets the original query apply offset/limit only once after sorting. I also added range-offset coverage to VertexCoreTest#testQueryByDateProperty to guard the double-skip case. Local checks passed with git diff --check, hugegraph-core compile, and VertexCoreTest#testQueryByDateProperty under the rocksdb core-test profile.

@contrueCT
contrueCT requested a review from imbajin June 24, 2026 10:15

@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: Found paging correctness regressions in the current head. Evidence: static review of head 262790 plus the failed hstore check.


🔗 Please also check the failed hstore job: https://github.com/apache/hugegraph/actions/runs/28083741191/job/83151751925

@contrueCT
contrueCT requested a review from imbajin July 12, 2026 13:21
@imbajin
imbajin requested a review from VGalaxies July 12, 2026 13:51

@VGalaxies VGalaxies 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.

🚨 Review summary

Important

Negative-label queries can return incomplete results when eligible labels have uneven index coverage.

📊 Risk dashboard

Signal Result
🚦 Review gate Blocked
🔎 Actionable findings 1 (1 P1)
🧪 Verification coverage 5 checks
⚠️ Confirmed / limited checks 1 / 2

🔬 Coverage details

ConditionQuery contract — Clear

Trace: collectConditionValues(), resolveConditionValues(), legacy condition() compatibility, direct callers

Conclusion: The reviewer traced the updated condition-resolution contract and its compatibility path and found no actionable API or caller regression.

⚠️ Label and index planning — Confirmed

Trace: GraphIndexTransaction, GraphTransaction, hasUserpropNeqCondition(), collectMatchedIndexes(), queryIndex()

Conclusion: The reviewer retained the partial-index false negative and dropped the separate conflicting-label claim because queryIndex() rejects multiple system-property conditions.

🟡 Backend compatibility — Limited

Trace: BinarySerializer, TextSerializer, RamTable, HstoreStore

Conclusion: The reviewer found no actionable serialization or backend compatibility defect in the changed paths, but local HStore runtime behavior could not be exercised.

ConditionQuery regression tests — Clear

Trace: QueryTest

Conclusion: QueryTest completed all 11 tests with zero failures or errors, confirming the direct condition-query behavior covered by that suite.

🟡 Cross-label coverage tests — Limited

Trace: six focused memory-core vertex and edge tests, HStore CI VertexCoreTest.testQueryByDateProperty

Conclusion: All six focused memory-core tests passed, but none creates a matching fan or follower vertex without equivalent city-index coverage; HStore CI also remains red on an unrelated date-property assertion.

Warning

Verification limits

  • The focused memory tests do not cover a matching fan or follower vertex whose city property lacks equivalent index coverage.
  • HStore CI fails in VertexCoreTest.testQueryByDateProperty with expected year 2013 but actual year 2014.
  • HStore runtime behavior was not exercised locally.

🤖 Codex review · GPT-5.6 Sol · effort: xhigh

@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: Three correctness risks remain in multi-label and mixed-label query planning, and latest-head CI is failing. Evidence: six independent static review lanes plus latest-head GitHub checks.

@VGalaxies VGalaxies 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.

🚨 Review summary

Important

Changes requested: multi-container negative-label filters still enter unsupported query validation, and the legacy condition documentation contradicts empty-IN behavior.

🧪 Verification matrix

Independent check What it traced Outcome
⚠️ Condition resolution
Confirmed
ConditionQuery condition accessors and flattening, GraphTransaction and GraphIndexTransaction label consumers, QueryTest coverage Confirmed that the new Javadoc says an empty resolved EQ/IN set returns null, while the sole-IN fast path returns the raw empty list; retained as a low-severity contract finding.
🧹 Backend execution
Dropped
GraphTransaction query optimization and edge sort-key routing, GraphIndexTransaction index planning, BinarySerializer, TextSerializer, RamTable, and HstoreStore The only reported issue matched the existing TraversalUtil mixed-connective comment at line 471, so it was dropped as a duplicate; no other distinct backend finding survived.
🟡 Traversal semantics
Limited
HugeGraphStep and HugeVertexStep label extraction, TinkerPop HasStep folding, VertexCoreTest and EdgeCoreTest label regressions No distinct finding was reported, but that conclusion was limited by the uncovered multi-container pure-label case subsequently demonstrated against the compiled HEAD classes.
⚠️ Targeted tests
Confirmed
QueryTest under the unit-test profile, Three changed VertexCoreTest methods under core-test,memory, Three changed EdgeCoreTest methods under core-test,memory QueryTest passed 10/10 tests. The targeted vertex and edge regressions passed 6/6 tests with zero failures, errors, or skips.
⚠️ Negative-label composition
Confirmed
TraversalUtil.canExtractHasContainers() at lines 446-459, TinkerPop HasStep construction for sequential label predicates, GraphTransaction.verifyVerticesConditionQuery() and verifyEdgesConditionQuery() TinkerPop formed one HasStep containing EQ and NEQ label containers. The compiled HEAD method returned false for a sole NEQ container but true for the EQ+NEQ pair; downstream validation rejects the resulting two-condition label-only query.

Warning

Verification limits

  • Runtime tests used the memory backend only; Cassandra, MySQL, RocksDB, and HStore profiles were not executed.
  • The multi-container finding has no end-to-end regression in the current suite; confirmation used the compiled HEAD extraction method and a downstream validation trace.

🤖 Codex review · GPT-5.6 Sol · effort: xhigh

@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: Retaining an unsafe label filter while extracting later property filters can silently omit valid matches; latest-head server checks are also failing. Evidence: six independent static review lanes plus latest-head GitHub Actions logs.

@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: An unsafe LABEL filter can still be separated from a later indexed property filter by a barrier, allowing the property filter to be pushed down into an incomplete index plan. Evidence: three independent static review lanes traced TraversalUtil across HugeGraphStep and HugeVertexStep; latest-head functional jobs pass, but codecov/project is failing.

…ition-query-semantics

# Conflicts:
#	hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtilOptimizeTest.java

@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: No new non-duplicate code issue was confirmed on this head, but the latest HStore check is failing, so this revision cannot be approved. Evidence: hstore job 88938722047 failed VertexCoreTest#testQueryByDateProperty (expected 2013, got 2015) and VertexCoreTest#testQueryByPropertyInPageWithLimitLtePageSize (undefined index label 1228551733); all other visible checks passed.

@contrueCT

contrueCT commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking the latest HStore failure. I traced the two reported errors separately:

The second failure is clearly intermittent and state-dependent:

  • July 12: the test failed with missing index-label ID 207.
  • July 18: it failed with ID 1228551550.
  • July 19: the same HStore workflow passed completely in run 29683719157.
  • July 22: it failed with ID 1228551733.

The changing dynamically allocated schema IDs, together with a complete passing run between the failures, are more consistent with schema deletion/recreation, cache propagation, or stale index data crossing test boundaries than with a deterministic regression in the ConditionQuery semantics. More specifically, this could be either a stale HStore index row or the current index label becoming temporarily unavailable during schema/cache convergence. The latest run did not upload the Surefire report or deeper diagnostics, so the current log cannot distinguish those two HStore sub-cases.

I also don't plan to restore the earlier broad “skip undefined index-label rows” workaround: as noted in the previous review, a fully skipped backend page can make PagingIdHolder stop early and miss later valid rows. Would it be reasonable to treat these two failures as the tracked HStore issues in #3053 and #3090, outside the scope of this PR?

@contrueCT
contrueCT requested a review from imbajin July 24, 2026 04:47
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Aug 5, 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: Four correctness issues remain in negative-label pushdown, and the exact head has visible CI failures. Evidence: static review of the exact-head production paths plus CountStrategyCoreTest#testNegativeConnectiveLabelAfterNoIndexRangeStaysLocal failing across the main backend jobs.

@contrueCT contrueCT changed the title fix(query): clarify condition resolution semantics for label queries fix(server): clarify condition resolution and ensure negative-label index completeness Aug 6, 2026
@contrueCT
contrueCT force-pushed the task/improve-condition-query-semantics branch from 61e5bbc to 1d004a3 Compare August 6, 2026 05:01
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Aug 6, 2026
@contrueCT contrueCT changed the title fix(server): clarify condition resolution and ensure negative-label index completeness fix(server): clarify condition resolution semantics for label queries Aug 6, 2026
@contrueCT

Copy link
Copy Markdown
Contributor Author

I will first create a separate PR to fix #3053 and merge it into this branch before continuing with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improve]: clarify ConditionQuery.condition() semantics for missing, conflicting, and multi-value conditions

3 participants