Quantexa arithmetic where#98
Conversation
Merging this PR will degrade performance by 24.9%
|
|
The performance changes - I'm not sure how much to worry about these. I think the extra time is all about the creation of the query (the queries can now be more complex and there is type-checking). As it's about the creation of the query, it won't really affect how long it takes to run the query at scale, which I think is what performance tests are usually about measuring? |
j6k4m8
left a comment
There was a problem hiding this comment.
so ID() conditions always fell through to IndexerUnsupportedOp, triggering a full scan.
omg!!! Good catch, that is such a great fix!
The performance changes - I'm not sure how much to worry about these.
Agreed, I don't think these numbers are concerning enough to offset the benefits of the new feature support! Unfortunately I suspect enriching the syntax will inevitably lead to performance hits. But my opinion is let's get feature-parity first and performance can come later; this is great!
The coercion is now type-aware: EntityRef, AttributeRef, and IDRef are preserved, everything else is converted to str.
This is great, and will improve return-handling considerably!!
Is this ready to merge?
RETURN and ORDER BY do still use the old string parsing approach for some stuff, but it should now be much easier for us to refactor that if we want to. With so much going on I think types can really help us. I can raise an issue for this refactor if you're interested in that, I decided not to make all those changes in this PR.
I'm very happy if this code is merged! Looks like the lint check failed for something to do with .venv? |
|
I'd love to refactor a bit to surface types better — if you are interested, that would be an amazing update! I am working on resolving the .venv issue now... no idea where that came from! |
| "-": lambda a, b: a - b, | ||
| "*": lambda a, b: a * b, | ||
| # Integer division to match Neo4j/Cypher semantics | ||
| "/": lambda a, b: a // b if isinstance(a, int) and isinstance(b, int) else a / b, |
There was a problem hiding this comment.
python // floor-rounds, so for example,
-3 // 2 == -2but I thiiiiiiiink java follows truncate-toward-zero, iirc:
-3 / 2 == -1I think this can be fixed with,
| "/": lambda a, b: a // b if isinstance(a, int) and isinstance(b, int) else a / b, | |
| "/": lambda a, b: int(a / b) if isinstance(a, int) and isinstance(b, int) else a / b, |
There was a problem hiding this comment.
Oooh great catch - I've checked division in Neo4j and you're right, I've updated the code with your suggestion, also with a comment to explain and a test.
|
Amazing, @q-rosiebloxsom — merging now! 🎉 |
Summary
Adds support for arithmetic operators (
+,-,*,/,%) in WHERE clauses, enabling queries like:Changes
Grammar (
_GrandCypherGrammar)The
conditionrule previously accepted onlyentity_id op entity_id_or_value. It now acceptsarith_expr op arith_expr, wherearith_expris a two-level precedence grammar:arith_expr— handles+/-(lower precedence)arith_term— handles*///%(higher precedence)arith_atom— leaf nodes:entity_id,scalar_function,value,NULL,TRUE,FALSE, or a parenthesized sub-expressionThe
INoperator intentionally keeps its old rule (entity_id op_list value_list) — arithmetic on the LHS ofINis not supported by design.Typed operand classes:
EntityRef,AttributeRef,IDRef,ArithmeticExpressionThe old code represented all entity references as plain strings (
"A","A.age") and ID lookups as string patterns ("ID(A)"). This made it impossible to distinguish operand types at evaluation time and required fragile string-parsing (e.g.startswith("ID("),split(".")) throughoutCompoundCondition.__call__and the indexer.Four typed classes replace this:
EntityRef(str)— bare node/edge reference, e.g.A. RaisesTypeErrorif used in a WHERE comparison, since bare entities aren't meaningful in comparisons.AttributeRef(str)— node/edge attribute reference, e.g.A.age, with.entity_nameand.attributeaccessors.IDRef(str)—ID(A)reference with.entity_nameaccessor.ArithmeticExpression— recursive tree node (left,op,right) with a.resolve()method for evaluation.All
strsubclasses retain string equality and dict-key compatibility.Operand resolution:
_resolve_operand()A single recursive function replaces the scattered resolution logic that was previously inlined in
CompoundCondition.__call__. It dispatches on operand type:ArithmeticExpression— recursively resolves via.resolve()IDRef— looks up the node ID from the match mappingAttributeRef— looks up the node/edge attribute from the host graphEntityRef— raisesTypeError(bare entities are not valid in comparisons)CompoundConditionrefactorConstructor parameters renamed from
entity_id/valuetoleft/rightto reflect that either side of a comparison can now be an arbitrary expression, not just an entity reference or a literal.The
__call__method is simplified from ~30 lines of branching string-inspection to a short delegation to_resolve_operand().Division semantics
Integer division (
//) is used when both operands are integers, matching Neo4j/Cypher semantics. Float division is used otherwise.Error handling
ArithmeticExpression.resolve()catchesTypeError,ZeroDivisionError, andOverflowError, returningNone.CompoundConditiontreatsNoneasFalse, so rows with non-numeric attributes or division by zero are silently excluded rather than crashing.Indexer (
to_indexer_astandCompare)to_indexer_astcreatesIndexerComparefor two patterns:AttributeRef op literal— the standard case, e.g.WHERE A.age > 20.Compare.__call__usesself._key.attributeandself._key.entity_namedirectly to query the node attribute index.IDRef == literal— e.g.WHERE ID(A) == 2.Compare.__call__returns{entity_name: {value}}directly, restricting grandiso to that single node ID. Only equality is supported; inequality operators onIDReffall through toIndexerUnsupportedOpfor full-scan evaluation.All other combinations fall through to
IndexerUnsupportedOp.motif_to_indexer_astusesAttributeRefwhen building indexer keys.Return path (
_return_requestscoercion)The old
list(map(str, self._return_requests))line stripped LarkTokenwrappers to plain strings. With typed operand classes now in play, a blanketstr()conversion destroys the type information that_lookuprelies on (e.g.isinstance(data_path, IDRef)). The coercion is now type-aware:EntityRef,AttributeRef, andIDRefare preserved, everything else is converted tostr.Bugfix:
ID()comparisons with inequality operatorsThe old
to_indexer_asthad a dead branch —entity_id.startswith("ID()")(note the empty parens) never matched actual strings like"ID(A)", soID()conditions always fell through toIndexerUnsupportedOp, triggering a full scan. This happened to produce correct results because the full-scan path evaluated the comparison properly.The dead branch was intended to strip
ID(A)down toAand pass it to the indexer'sCompareclass, where the single-element key path (key with no dot) would handle it. However, thatComparepath also had a latent bug: it ignored the operator entirely and treated every comparison as equality. For example,WHERE ID(A) > 1on a graph with nodes 1, 2, 3 would pass key"A", op">", value1toCompare. The single-element branch would ignore">"and return{"A": {1}}— telling the executor to only consider node 1 for entity A. Nodes 2 and 3 would never be evaluated. The WHERE condition would then check1 > 1on the sole candidate, which is false, producing an empty result set.We have removed both the dead branch in
to_indexer_astand the single-element key path inCompare, since neither was ever reached and the latter doesn't work correctly.ID()conditions are now handled by the typed operand approach:to_indexer_astroutesIDRef == literaltoIndexerComparefor indexed lookup, preserving the==optimisation that the original code intended. Inequality operators likeWHERE ID(A) > 1, which would have produced wrong results if the dead branch had been reached, are routed toIndexerUnsupportedOpfor correct full-scan evaluation.Known issue: bare entity names in WHERE
test_bare_entity_name_in_where_should_erroris markedxfail. A query likeWHERE A + 0 > 1(using a bare entity nameArather than an attribute likeA.age) should raise aTypeError, but currently silently returns empty results. This happens because the?prefix on theentity_idgrammar rule inlines bareCNAMEtokens directly, bypassing theentity_idtransformer method. The bareAarrives as a raw LarkTokenrather than anEntityRef, so_resolve_operandtreats it as a literal string value instead of raising. Fixing this would require either removing the?prefix (which affects all grammar rules that referenceentity_id) or adding a validation pass after transformation. This was an existing issue which is not yet fixed.Tests
test_queries.py— 23 new end-to-end tests covering all operators, precedence, parentheses, edge attributes,ID()equality and inequality, chained expressions, error resilience (non-numeric, division by zero, missing attributes),AND/NOTcombinations, and cross-entity arithmetic.test_parser.py— 10 new parser tests verifying parse tree structure (correctarith_*node types, operator nodes, literal values), not just that parsing doesn't crash.test_to_indexer_ast.py— 9 new tests covering positive routing (AttributeRef+ literal →IndexerCompare,IDRef+==+ literal →IndexerCompare) and negative routing (bareEntityRef, swapped operands,ArithmeticExpressionon either side,IDRefwith inequality →UnsupportedOp).test_indexer_plumbing.py— 7 new plumbing tests that mockgrandiso.find_motifs_iterto verify the indexer produces correct hints end-to-end: attribute equality/inequality, entity scoping,ID()equality producing hints,ID()inequality/arithmetic/edge attributes falling back to no hints.test_indexer.py— updatedComparetests to useAttributeRefkeys instead of plain strings.