Skip to content

Wire Neo4j heuristics into the search fitness function - #1731

Merged
arcuri82 merged 11 commits into
masterfrom
neo4j-fitness-integration
Sep 9, 2026
Merged

Wire Neo4j heuristics into the search fitness function#1731
arcuri82 merged 11 commits into
masterfrom
neo4j-fitness-integration

Conversation

@andyfelder16

@andyfelder16 andyfelder16 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Last of the PRs splitting #1641, on top of #1580, #1672, #1647, #1697 and #1715, all of which are merged. It closes the chain: the Cypher queries the SUT runs have been captured since #1460, and this is what finally turns them into search guidance.

  • Neo4jHandler collects the queries intercepted from Session.run, reads the graph once per action rather than once per query, and scores each query against that snapshot. Only MATCH queries are scored; anything that does not parse as one, such as a write, is skipped
  • Neo4jCommandWithDistance and Neo4jDistanceWithMetrics carry the result to the DTO
  • EnterpriseFitness.handleNeo4jHeuristics feeds the distances to the search, behind the new heuristicsForNeo4j option, and reports evaluation counts and the average number of nodes inspected through Statistics
  • Nothing throws back at the SUT: a driver that cannot be queried, a query that does not parse, and a scoring failure each degrade to no heuristic or to a failed evaluation, and the search carries on
  • 5 handler tests plus one in StatisticsTest, covering the scored path, a write being skipped, no driver, heuristics disabled, and a driver that cannot be reached

@andyfelder16
andyfelder16 marked this pull request as ready for review September 1, 2026 14:59
* @param evaluationFailure whether the evaluation failed
*/
public Neo4jDistanceWithMetrics(double distance, int numberOfEvaluatedNodes, boolean evaluationFailure) {
if (distance < 0.0d || distance > 1.0d || Double.isNaN(distance)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @andyfelder16 .thx ;) but should first request @jgaleotti review, and, then, once that is completed, JP asks for my review. anyway, with a quick look here i can see the naming is confusing. see DistanceHelper for a definition of distance vs heuristic inside EM codebase

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arcuri82 , I am taking over the review. However, this is indeed a distance since it follows the same model as MongoDistanceWithMetrics, RedisDistanceWithMetrics, DynamoDbDistanceWithMetrics and CqlDistanceWithMetrics (i.e., Cassandra)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is distance, why are we throwing an exception if greater than 1? this is confusing. if it is a normalized distance, then it should be explicitely called that, eg, double normalizedDistance

* @param evaluationFailure whether the evaluation failed
*/
public Neo4jDistanceWithMetrics(double distance, int numberOfEvaluatedNodes, boolean evaluationFailure) {
if (distance < 0.0d || distance > 1.0d || Double.isNaN(distance)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arcuri82 , I am taking over the review. However, this is indeed a distance since it follows the same model as MongoDistanceWithMetrics, RedisDistanceWithMetrics, DynamoDbDistanceWithMetrics and CqlDistanceWithMetrics (i.e., Cassandra)

metrics = new Neo4jDistanceWithMetrics(distance, graph.nodeCount(), false);
} catch (Exception e) {
SimpleLogger.uniqueWarn("Failed to compute Neo4j heuristic for query: " + query);
metrics = new Neo4jDistanceWithMetrics(1.0, graph.nodeCount(), true);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace 1.0 with Neo4HeuristicsCalculator.MAX_NEO4J_DISTANCE = 1.0d

@jgaleotti jgaleotti Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the value of MAX_NEO4J_DISTANCE should be Double.MAX_VALUE instead of 1.0d

.toList()

if (toMinimize.isNotEmpty()) {
fv.setExtraToMinimize(i, toMinimize)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is behind master

@jgaleotti

Copy link
Copy Markdown
Collaborator

@andyfelder16 please update branch and fix conflicts

@jgaleotti
jgaleotti requested a review from arcuri82 September 7, 2026 16:11
* @param evaluationFailure whether the evaluation failed
*/
public Neo4jDistanceWithMetrics(double distance, int numberOfEvaluatedNodes, boolean evaluationFailure) {
if (distance < 0.0d || distance > 1.0d || Double.isNaN(distance)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is distance, why are we throwing an exception if greater than 1? this is confusing. if it is a normalized distance, then it should be explicitely called that, eg, double normalizedDistance

*/
public Neo4jDistanceWithMetrics(double distance, int numberOfEvaluatedNodes, boolean evaluationFailure) {
if (distance < 0.0d || distance > 1.0d || Double.isNaN(distance)) {
throw new IllegalArgumentException("distance must be between 0 and 1, but was " + distance);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a distance does not need to be within 0 and 1. if it is a normalized distance, then explicitely state so, eg, a normalized distance must be between 0 and 1,

/**
* Creates a Neo4j heuristic result.
*
* @param distance normalized distance to satisfying the query, 0 meaning satisfied

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason why Neo4j works on normalized distances?

/**
* @return normalized distance to satisfying the query, 0 meaning satisfied
*/
public double getDistance() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if normalized, then the name getDistance is confusing.

try {
neo4jHandler.handle(it);
} catch (Exception e){
SimpleLogger.error("FAILED TO HANDLE NEO4J COMMAND");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add the e.getMessage to the this error log

* cannot compute a distance (for example when the evaluation throws) reports this value, so a failure
* never looks closer to satisfied than a genuine miss.
*/
public static final double MAX_NEO4J_DISTANCE = 1.0d;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does Neo4J require this bound, where all other systems do not? note that normalizing distances do lose information. that is why in DistanceHelper there are a lot of different helper functions to deal then a maxed distance is Double.MAX_VALUE

@arcuri82 arcuri82 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved by mistake, there are comments to address first

@jgaleotti

Copy link
Copy Markdown
Collaborator

hi @arcuri82 , the term normalized came from the fact that the Neo4JHeuristicsCalculator returns a Truthness that is in the [0..1] range. The distance is then calculated as (1.0d - truthness.ofTrue), which returns a value between [0..1].
Should we simply drop the normalized term and checks for Neo4jDistanceWithMetrics?

@arcuri82

arcuri82 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

@jgaleotti what do you mean with checks for Neo4jDistanceWithMetrics??

@arcuri82

arcuri82 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

@jgaleotti if the distance is computed from (1.0d - truthness.ofTrue), then it is fine. but still should remove any check on >1, or, alternative, explicitely state in the variable names that those are normalized within 1. otherwise it is quite confusing with the rest of the codebase

@jgaleotti

Copy link
Copy Markdown
Collaborator

please @andyfelder16 remove the checks that constrain the Neo4J distance to be <1. Also, replace 1.0d with Double.MAX_VALUE as the MAX_NEO4J_DISTANCE

@jgaleotti
jgaleotti requested a review from arcuri82 September 8, 2026 13:48
@arcuri82
arcuri82 merged commit 7022032 into master Sep 9, 2026
53 of 54 checks passed
@arcuri82
arcuri82 deleted the neo4j-fitness-integration branch September 9, 2026 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants