bitflicker64 commented on code in PR #2994:
URL: https://github.com/apache/hugegraph/pull/2994#discussion_r3947216107
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/traversal/optimize/TraversalUtil.java:
##########
@@ -645,14 +667,327 @@ private static boolean
extractHasContainers(HugeVertexStep<?> newStep,
private static boolean canExtractHasContainers(HugeGraph graph,
HasContainerHolder holder) {
- for (HasContainer has : holder.getHasContainers()) {
+ // Keep unsafe labels and their sibling properties for local filtering.
+ if (hasUnsafeLabelPredicate(holder)) {
+ return false;
+ }
+ List<HasContainer> hasContainers = holder.getHasContainers();
+ for (HasContainer has : hasContainers) {
if (!canExtractHasContainer(graph, has)) {
return false;
}
}
return true;
}
+ private static void prepareLocalHasContainers(
+ Step<?, ?> source, Traversal.Admin<?, ?> traversal) {
+ QueryHolder query = (QueryHolder) source;
+ Step<?, ?> step = source.getNextStep();
+ while (step instanceof HasStep || step instanceof NoOpBarrierStep) {
+ Step<?, ?> next = step.getNextStep();
+ if (step instanceof HasStep) {
+ HasContainerHolder holder = (HasContainerHolder) step;
+ for (HasContainer has : new
ArrayList<>(holder.getHasContainers())) {
+ // Paging is query metadata, never an element property
filter.
+ if (QueryHolder.SYSPROP_PAGE.equals(has.getKey())) {
+ query.addHasContainer(has);
+ holder.removeHasContainer(has);
+ continue;
+ }
+ if (T.id.getAccessor().equals(has.getKey())) {
+ // ID lookup is complete across labels. Existing source
+ // IDs and unsupported predicates still filter locally.
+ if (source instanceof HugeGraphStep &&
+ GraphStep.processHasContainerIds((HugeGraphStep<?,
?>) source, has)) {
+ holder.removeHasContainer(has);
+ continue;
+ }
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new HasContainer(has.getKey(),
+ localIdPredicate(has.getPredicate())));
+ continue;
+ }
+ if (T.label.getAccessor().equals(has.getKey())) {
+ holder.removeHasContainer(has);
+ holder.addHasContainer(new
LocalLabelHasContainer(has.getPredicate()));
+ continue;
+ }
+ if (isSysProp(has.getKey())) {
Review Comment:
‼️ `hasKey()` and `hasValue()` silently return nothing on this path.
`~page`, `T.id` and `T.label` have all been handled and `continue`d above,
and `isSysProp` (line 1545) resolves through `token2HugeKey`, so the only keys
that reach this branch are `T.key` (`"key"`) and `T.value` (`"value"`). They
stay in the local `HasStep` untouched.
On the extraction path they are pushed down: `canExtractHasContainer`
returns `true` for every sysprop key (line 993) and `convContains2Relation`
(line 1403) turns them into `Condition.containsKey(PROPERTIES, pkId)` /
`containsValue(...)`. That is what makes `g.V().hasKey("age")` work today
(`VertexCoreTest:3419`, `EdgeCoreTest:2884`).
Left local, TinkerPop's `HasContainer.test(Element)` (gremlin-core 3.5.1)
special-cases only `T.id` and `T.label` and otherwise falls through to
`element.properties(this.key)`, so the container is evaluated as a property
literally named `"key"`. `HugeVertex.properties(String...)` (line 573) catches
the `IllegalArgumentException` from `graph().propertyKey("key")` and continues,
so the iterator is empty and the predicate is false for every element.
Result: `g.V().hasKey("age").hasLabel(P.neq("person"))` returns nothing
instead of the vertices that have an `age` property, with no error. Same for
`hasValue(...)` and for the `HugeVertexStep` variant. I confirmed the plan
difference: `__.V().hasKey("age")` moves the `T.key` container into
`HugeGraphStep` and drops the local `HasStep`, while
`__.V().hasKey("age").hasLabel(P.neq("other"))` leaves it in the `HasStep` with
its original `eq(age)` predicate.
Please handle `T.key` and `T.value` explicitly here, the way `T.id` and
`T.label` are handled just above. They are label-independent, so pushing them
into the source query stays complete. A regression test for `hasKey(...)` next
to a negative label would pin this.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/HugeGraph.java:
##########
@@ -289,6 +290,9 @@ public interface HugeGraph extends Graph {
<K, V> V option(TypedOption<K, V> option);
+ // Build the same term matcher used by SEARCH indexes, without reading
data.
+ Predicate<Object> searchPredicate(String text);
Review Comment:
🧹 Adding an abstract method to the published `HugeGraph` interface forces
every out-of-tree implementor to recompile, and an implementation that is not
recompiled fails with `AbstractMethodError` once this method is called. In-tree
only `StandardHugeGraph` and `HugeGraphAuthProxy` implement the interface, so
the build surfaces nothing.
The interface already carries nine `default` methods, so a `default`
throwing `UnsupportedOperationException` would keep third-party implementations
compiling. Note that a `default` cannot delegate to
`GraphIndexTransaction.searchPredicate(this.analyzer(), text)`, because
`analyzer()` is private to `StandardHugeGraph` and is not on the interface.
Please either make it `default`, or confirm the break is intended for this
release.
--
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]
