imbajin commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3105762290
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/tx/GraphTransaction.java:
##########
@@ -1966,6 +1966,14 @@ private boolean rightResultFromIndexQuery(Query query,
HugeElement elem) {
return false;
}
+ private static Id uniqueLabel(ConditionQuery query) {
+ Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
+ if (labels.size() != 1) {
+ return null;
+ }
+ return (Id) labels.iterator().next();
+ }
Review Comment:
‼️ **Code Duplication: `uniqueLabel()` duplicated in 4 files — consider
consolidating**
The `uniqueLabel(ConditionQuery)` helper is copy-pasted into 4 files with
identical bodies:
- `GraphTransaction.java` (private static)
- `GraphIndexTransaction.java` (private static)
- `RamTable.java` (private static)
- `HstoreStore.java` (private **instance** method — inconsistent with the
other 3)
I understand these callers need "return null when multiple labels exist"
semantics (unlike `conditionValue()` which throws). Two suggestions:
**Option A** — Add this as a first-class method on `ConditionQuery` (e.g.,
`uniqueConditionValue(Object key)` or `singleConditionValueOrNull(Object key)`)
to eliminate all 4 copies:
```java
public <T> T uniqueConditionValue(Object key) {
Set<Object> values = this.conditionValues(key);
if (values.size() != 1) {
return null;
}
@SuppressWarnings("unchecked")
T value = (T) values.iterator().next();
return value;
}
```
**Option B** — If you prefer not to expand the `ConditionQuery` API surface,
at minimum consolidate to a `static` utility in one shared location (e.g., a
package-visible helper in `ConditionQuery` or a `QueryUtil`).
Also note: the `HstoreStore` version is an instance method while the other 3
are `static` — this inconsistency should be fixed either way.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BinarySerializer.java:
##########
@@ -674,7 +674,7 @@ private Query writeQueryEdgeRangeCondition(ConditionQuery
cq) {
if (direction == null) {
direction = Directions.OUT;
}
- Id label = cq.condition(HugeKeys.LABEL);
+ Id label = cq.conditionValue(HugeKeys.LABEL);
Review Comment:
⚠️ **Inconsistent migration: mixed `conditionValue()` vs `uniqueLabel()` for
LABEL key**
In this PR, some callers migrate to `conditionValue(HugeKeys.LABEL)`
(serializers, traverser), while others use the new private `uniqueLabel()`
helper (transactions, RamTable, HstoreStore). The two have **different failure
behavior** for multi-label queries:
- `conditionValue()` → **throws** `IllegalStateException` if multiple values
remain
- `uniqueLabel()` → returns **null** silently
Is this difference intentional per call site? If so, a brief comment at each
call site explaining why that particular semantic was chosen would help future
readers. Otherwise, consider unifying to a single approach — either all callers
that need "single or nothing" use one API, and those that need "single or fail"
use the other, with the distinction documented.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/query/ConditionQuery.java:
##########
@@ -323,20 +325,110 @@ public <T> T condition(Object key) {
return value;
}
+ /**
+ * Returns whether there is any top-level relation for the specified key.
+ */
+ public boolean containsCondition(Object key) {
+ for (Condition c : this.conditions) {
+ if (c.isRelation()) {
+ Condition.Relation r = (Condition.Relation) c;
+ if (r.key().equals(key)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Returns the resolved candidate values of the specified key from
+ * top-level EQ/IN relations.
+ *
+ * Use {@link #containsCondition(Object)} to distinguish "no condition"
+ * from "conditions exist but resolve to an empty intersection".
+ */
+ public Set<Object> conditionValues(Object key) {
+ List<Object> valuesEQ = InsertionOrderUtil.newList();
+ List<Object> valuesIN = InsertionOrderUtil.newList();
+ this.collectConditionValues(key, valuesEQ, valuesIN);
+ if (valuesEQ.isEmpty() && valuesIN.isEmpty()) {
+ return InsertionOrderUtil.newSet();
+ }
+ return this.resolveConditionValues(valuesEQ, valuesIN);
+ }
+
+ /**
+ * Returns the unique resolved value of the specified key from top-level
+ * EQ/IN relations.
+ *
+ * Returns {@code null} when the resolved candidate set is empty. Throws
+ * if multiple values remain after resolution.
+ */
+ public <T> T conditionValue(Object key) {
+ Set<Object> values = this.conditionValues(key);
+ if (values.isEmpty()) {
+ return null;
+ }
+ E.checkState(values.size() == 1,
+ "Illegal key '%s' with more than one value: %s",
+ key, values);
+ @SuppressWarnings("unchecked")
+ T value = (T) values.iterator().next();
+ return value;
+ }
+
public void unsetCondition(Object key) {
this.conditions.removeIf(c -> c.isRelation() && ((Relation)
c).key().equals(key));
}
public boolean containsCondition(HugeKeys key) {
+ return this.containsCondition((Object) key);
+ }
+
+ private void collectConditionValues(Object key, List<Object> valuesEQ,
+ List<Object> valuesIN) {
for (Condition c : this.conditions) {
if (c.isRelation()) {
Condition.Relation r = (Condition.Relation) c;
if (r.key().equals(key)) {
- return true;
+ if (r.relation() == RelationType.EQ) {
+ valuesEQ.add(r.value());
+ } else if (r.relation() == RelationType.IN) {
+ Object value = r.value();
+ assert value instanceof List;
+ valuesIN.add(value);
+ }
}
}
}
- return false;
+ }
+
+ private Set<Object> resolveConditionValues(List<Object> valuesEQ,
+ List<Object> valuesIN) {
+ boolean initialized = false;
Review Comment:
⚠️ **`resolveConditionValues` duplicates logic already in `condition()` —
consider extracting the shared intersection**
The new `resolveConditionValues()` method contains the same intersection
logic as the existing `condition()` method. The only difference is
`condition()` has extra fast-path returns and the final single-value check.
Since `condition()` now delegates `collectConditionValues()` to the new
private method (good!), it could also delegate the intersection to
`resolveConditionValues()` to fully eliminate the duplicated loop:
```java
public <T> T condition(Object key) {
List<Object> valuesEQ = InsertionOrderUtil.newList();
List<Object> valuesIN = InsertionOrderUtil.newList();
this.collectConditionValues(key, valuesEQ, valuesIN);
if (valuesEQ.isEmpty() && valuesIN.isEmpty()) {
return null;
}
// Keep legacy fast paths for backward compatibility
if (valuesEQ.size() == 1 && valuesIN.isEmpty()) { ... }
if (valuesEQ.isEmpty() && valuesIN.size() == 1) { ... }
// Delegate to shared intersection instead of duplicating
Set<Object> intersectValues = this.resolveConditionValues(valuesEQ,
valuesIN);
// ... rest of legacy checks
}
```
This would make `condition()` and `conditionValues()` share the same
intersection code path, reducing divergence risk.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/serializer/BinarySerializer.java:
##########
@@ -722,7 +722,8 @@ private Query writeQueryEdgePrefixCondition(ConditionQuery
cq) {
int count = 0;
BytesBuffer buffer = BytesBuffer.allocate(BytesBuffer.BUF_EDGE_ID);
for (HugeKeys key : EdgeId.KEYS) {
- Object value = cq.condition(key);
+ Object value = key == HugeKeys.LABEL ?
+ cq.conditionValue(key) : cq.condition(key);
Review Comment:
⚠️ **Ternary for LABEL-only routing is fragile**
This introduces a special-case ternary:
```java
Object value = key == HugeKeys.LABEL ?
cq.conditionValue(key) : cq.condition(key);
```
The iteration is over `EdgeId.KEYS` (OWNER_VERTEX, DIRECTION, LABEL,
SORT_VALUES, OTHER_VERTEX), and only LABEL gets the new API — a fact that's not
self-documenting. If another key eventually needs the same treatment, a reader
must remember to add it here too.
Consider either:
1. Adding a brief inline comment explaining *why* only LABEL needs
`conditionValue()` (e.g., "LABEL may have multiple IN conditions; other EdgeId
keys are always single-valued"), or
2. Applying `conditionValue()` uniformly for all keys in this loop if the
semantics are safe (since the other keys are guaranteed single-valued,
`conditionValue` would behave identically to `condition` for them)
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/backend/store/ram/RamTable.java:
##########
@@ -377,6 +377,14 @@ private static void ensureNumberId(Id id) {
}
}
+ private static Id uniqueLabel(ConditionQuery query) {
+ java.util.Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
Review Comment:
🧹 **Minor: `RamTable.uniqueLabel` uses fully-qualified `java.util.Set`
instead of import**
```java
private static Id uniqueLabel(ConditionQuery query) {
java.util.Set<Object> labels = query.conditionValues(HugeKeys.LABEL);
```
The other 3 copies of `uniqueLabel()` use the imported `Set`. This one uses
`java.util.Set` — likely a missing import. Trivial, but worth cleaning up for
consistency (or moot if the duplication is resolved per the earlier comment).
--
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]
