VGalaxies commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3590250351
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/ConditionQuery.java:
##########
@@ -256,24 +256,27 @@ public boolean containsLabelOrUserpropRelation() {
return false;
}
+ /**
+ * Returns the legacy condition value of the specified key.
+ *
+ * This method keeps the historical behavior for existing callers:
+ * <ul>
+ * <li>returns {@code null} if no top-level EQ/IN relation exists</li>
+ * <li>returns {@code null} if top-level EQ/IN relations resolve to
empty</li>
Review Comment:
### 💡 Low · Clarify the empty-IN legacy contract
#### 🔄 Contract mismatch
| Expected behavior | Actual behavior |
| --- | --- |
| The documentation precisely describes the preserved result for a sole
empty IN relation. | The documentation promises null while the implementation
returns an empty List. |
The new Javadoc says condition() returns null when top-level EQ/IN relations
resolve to an empty set, but the sole-IN fast path at lines 288-291 returns the
raw list before resolution. A single `LABEL IN []` therefore returns an empty
List, not null.
> [!TIP]
> **Impact:** The documented contract contradicts the preserved legacy
behavior, which can mislead callers selecting among condition(),
conditionValues(), and conditionValue(). The boundary is also absent from
QueryTest.
#### 🛠️ Suggested change
Document that a sole IN relation returns its raw list even when empty, and
add a QueryTest assertion for `LABEL IN []` to pin the legacy behavior.
---
<sub>🤖 Codex review · **GPT-5.6 Sol** · effort: **xhigh**</sub>
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -445,6 +445,12 @@ private static boolean
extractHasContainers(HugeVertexStep<?> newStep,
private static boolean canExtractHasContainers(HugeGraph graph,
HasContainerHolder holder) {
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ // Keep pure label non-EQ/IN predicates on GraphStep for TinkerPop
filtering.
+ if (hasContainers.size() == 1 &&
Review Comment:
### ⚠️ Medium · Retain multi-container negative-label filters
#### 🔄 Contract mismatch
| Expected behavior | Actual behavior |
| --- | --- |
| A pure-label conjunction containing NEQ or WITHOUT remains a TinkerPop
filter and returns elements satisfying every label predicate. | Only a
single-container holder is retained; a multi-container holder is extracted and
rejected by condition-query validation. |
TinkerPop represents `hasLabel("person").has(T.label, P.neq("author"))` as
one HasStep containing two label containers. Against the compiled HEAD classes,
canExtractHasContainers() returned false for one NEQ container but true for
this EQ+NEQ pair because the guard applies only when size() == 1. Both
relations are consequently moved into ConditionQuery, where GraphTransaction
rejects the two label-only sysprop conditions.
> [!WARNING]
> **Impact:** Valid vertex and edge traversals containing sequential
positive and negative label filters throw an unsupported-query exception
instead of returning the label intersection. Two separate negative-label
filters fail for the same reason.
#### 🛠️ Suggested change
Classify pure-label holders independently of their container count: retain
the holder for TinkerPop filtering whenever any contained label predicate is
not safely expressible as EQ/IN. Add vertex and edge regressions for sequential
EQ+NEQ and NEQ+NEQ label filters.
---
<sub>🤖 Codex review · **GPT-5.6 Sol** · effort: **xhigh**</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
