tkobayas commented on PR #6584:
URL:
https://github.com/apache/incubator-kie-drools/pull/6584#issuecomment-3858872399
Great work, @Rikkola . TBH, I was about to comment only the test coverage,
but Claude Code provided useful suggestions, so sharing below. I'm not yet
understanding well about the implementation details, but the suggestions seem
to be valid (Interestingly, there are only few overlap with the Copilot's
suggestions). Of course, you may split PRs to deal with the points. Thanks!
----
# Concerns for Commit 51064ca77e — BiLinear Joins
## Correctness Issues
### 1. Topological sort is inverted (`RuleOrderOptimizer`)
The sort builds consumers before providers — the opposite of what's
documented and intended. In `buildDependencyGraph()` (line 96),
`graph[consumer] = {providers}`. The in-degree loop (line 130) then increments
*providers'* in-degrees, causing *consumers* (in-degree 0) to be scheduled
first. The docstring at line 83 explicitly says "providerRule MUST build before
consumerRule."
This works by accident because `BiLinearJoinNode.linkOutsideLeftInput()` is
called lazily when the provider is eventually built, and all rules are built
before any session fires. But the algorithm does the opposite of what it claims.
### 2. `BiLinearTuple.size()` is off by one
The constructor (line 68-69) always overrides `this.handle` with
`secondNetworkTuple.getFactHandle()`, even though the caller passes `null` for
`rightFactHandle`. Then `size()` (line 210-215) counts `this.handle` as a
separate element:
```java
int rightSize = this.handle != null ? 1 : 0;
return firstSize + secondSize + rightSize;
```
This double-counts the second network's last fact. For `first=[A,B]` and
`second=[C,D]`, `size()` returns 5 instead of 4. This causes `toObjects()`,
`toFactHandles()`, and the virtual parent chain to include a duplicate entry.
### 3. `BiLinearTuple.toObjects(reverse)` is broken
When `reverse=true`, the method calls `firstNetworkTuple.toObjects(reverse)`
and `secondNetworkTuple.toObjects(reverse)` (getting individually-reversed
sub-arrays), concatenates them, then reverses the whole array again. This
produces wrong output. For `first=[A,B], second=[C,D]`: individually reversed =
`[B,A,D,C]`, combined then reversed again = `[C,D,A,B]` instead of the correct
`[D,C,B,A]`.
## Performance Issues
### 4. `getWrappedContext()` allocates on every constraint evaluation
`BiLinearBetaConstraints.getWrappedContext()` (line 178) calls
`wrappedConstraints.createContext()` on every invocation. This is called from
`isAllowedCachedLeft()`/`isAllowedCachedRight()`, which execute inside the
nested-loop join for every tuple pair. This is an allocation-heavy hot path for
an optimization feature that's supposed to improve performance.
### 5. No index support on the join
`PhreakBiLinearJoinNode` always does a full nested-loop scan —
`memory.fastIterator()` iterating all tuples. Standard `PhreakJoinNode` uses
hash-indexed joins when constraints support it. For N left tuples and M right
tuples, this is O(N*M) vs the standard O(N) or O(N+M) with indexing.
### 6. `System.getProperty()` called on every routing check
`BiLinearDetector.isBiLinearEnabled()` calls `System.getProperty()` on every
invocation. This is called from
`BiLinearRoutingHelper.shouldRouteToBiLinearRightMemory()`, which runs in
`processPeers()` for every segment propagation — even when BiLinear is
disabled. This adds overhead to every rule evaluation in every Drools
deployment.
## Test Issues
### 7. No cross-network constraint tests
All 14 tests use unconstrained patterns like `A(), B(), C(), D()`. There are
zero tests with actual cross-network beta constraints (e.g., `A($val : value)
B() C(value > $val) D()`). The constraint machinery in
`BiLinearBetaConstraints`/`BiLinearContextEntry` — the most complex part of the
implementation — is effectively untested.
### 8. `testBiLinearSayAAAAAAAAA` doesn't test what it claims
The test is supposed to validate the "all same type" exclusion logic in
`BiLinearDetector.isSameTypeChain()`. But line 350 sets
`drools.bilinear.enabled` to `false`, so BiLinear is entirely disabled. The
test passes because the feature is off, not because the same-type detection
works.
## Scope Limitations
### 9. Alpha constraints completely excluded
`BiLinearDetector.hasAlphaConstraints()` (line 189) excludes any rule with
**any** alpha constraint from BiLinear optimization. Most real-world rules have
alpha constraints (e.g., `Person(age > 18)`). This makes the optimization
inapplicable to the vast majority of production rule sets.
### 10. `extractDeclarations()` returns empty map
`BiLinearDeclarationContext.extractDeclarations(LeftTupleSource)` (line
138-139) always returns an empty `HashMap`. The constructor that takes
`LeftTupleSource` parameters (used by
`BiLinearJoinNode.createDeclarationContext()` at line 75) therefore builds
empty declaration maps. Declarations work only because
`BiLinearDeclarationOffsetHelper` handles them separately at the terminal node
level, but this makes `BiLinearDeclarationContext` partially dead code.
### 11. No rule removal handling
If a provider rule is removed from the KieBase, the consumer's
`BiLinearJoinNode` retains a dangling reference to the deleted provider's
network via `BiLinearLeftInputWrapper.wrappedSecondLeftInput`. There's no
unlink or cleanup path.
### 12. No NOT/EXISTS/OR/eval support
Only flat AND-group patterns are eligible. Rules with negation, existential
quantification, disjunction, or `eval()` are excluded. Combined with the alpha
constraint exclusion, the optimization applies only to rules with pure
type-only patterns in a flat AND — a very narrow slice of real-world usage.
--
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]
