fix(engine) #5629: an explicit null in an optional argument propagates - #5699
Conversation
Cypher functions with an optional trailing argument read an explicit null
in that position as "argument omitted, use the default", while the same
null in the first position propagated. normalize(null) answered null but
normalize('x', null) normalized as NFC, so the same absent value meant two
different things depending on which position it landed in.
Settled in favour of propagation, on three grounds:
- Neo4j documents that reading for every optional argument it defines one
for: round() "returns null if any of its input parameters are null", and
the same is said of replace()'s limit and btrim()'s trim character.
- ArcadeDB had already decided it this way once, for CypherSubstringFunction
in #5193, citing Neo4j - it just was not written anywhere the next
function would find it.
- 9 of the 22 optional-argument functions already propagated, so defaulting
was the drift, not the house rule.
The rule is stated once, on CypherFunctionHelper.isExplicitNull(), so the
next function follows it instead of re-deciding - which is how the arity
declarations drifted in #5484 and how normalize() and isNormalized() came to
disagree in #5602.
Eleven functions adopt it: the five named in the issue plus the five
*.truncate functions and SubstringFunction, all found by auditing every
function whose getMinArgs() differs from getMaxArgs(). Omitting the argument
still selects the default in every one of them.
Propagation is placed after the existing argument checks, so round(null, 2,
'SIDEWAYS') still reports the unusable mode and a bad truncate unit is still
reported rather than masked by the null.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
🟢 Coverage 100.00% diff coverage · -7.54% coverage variation
Metric Results Coverage variation ✅ -7.54% coverage variation Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (83448ea) 150078 114508 76.30% Head commit (f1b2f25) 182261 (+32183) 125314 (+10806) 68.76% (-7.54%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5699) 20 20 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Review: issue #5629 - explicit null in an optional argument propagates Reviewed the full diff. This is a high-quality, tightly-scoped change. The strongest part is that the decision is made ONCE on CypherFunctionHelper.isExplicitNull(args, position) and called rather than re-litigated per function, which is exactly the anti-drift mechanism the issue asked for. The Neo4j sourcing, the audit of all 22 optional-argument functions, and the ordering guarantee (propagation placed after argument validation) are all convincing and, importantly, pinned by tests. WHAT IS DONE WELL
MINOR OBSERVATIONS (non-blocking)
VERDICT No correctness or performance concerns. The change is internally consistent, well-reasoned, and the regression suite (CypherOptionalArgumentNullIssue5629Test, plus the two updated assertions) captures both the new behaviour and the ordering invariant. LGTM. |
… block The five *.truncate files had it above the other com.arcadedb.* imports and separated by a blank line. Pre-existing rather than introduced here - this commit's parent changed no import lines in those files - but they are being touched anyway, so tidy it rather than leave it for the next reader.
|
Thanks - all three observations checked. One applied, two noted with a correction to each. 1. Import placement - applied (28f6464), with one correction: the misplaced import was not introduced by this PR. The parent commit's diff for those five files is 2. Redundant post-check guards - no change, but the redundancy is narrower than described. After
What 3. Truncate family 500-on-client-error - agreed, and it is worse than the null-unit NPE alone. There are two distinct paths, both recorded as follow-up 1 in the tracking doc:
The second is why |
Code ReviewReviewed the full diff. This is an unusually strong PR: it treats a class of behavioral inconsistency rather than the five functions the issue named, states the rule in exactly one place (CypherFunctionHelper.isExplicitNull), and backs the decision with three independent lines of evidence (Neo4j docs, the prior #5193 precedent, and an audit of all 22 optional-argument functions). The write-up and the docs file make the reasoning auditable. Nice work. What I verified
Observations (all minor, none blocking)
The follow-ups you deliberately deferred (raw NullPointerException on a null unit, TrimFunction's mode exposure, the trim/ltrim null trim-char disagreement, and deciding whether to wire up or delete the second SubstringFunction) are all well-scoped and correctly kept out of this change. Item 5 in particular - two implementations of one function where only one is registered - is worth its own issue so the divergence does not silently reappear. LGTM. |
placement The four truncate omitted-argument assertions checked only isNotNull(), which the default path could satisfy while doing the wrong thing. They now assert the concrete truncated value, as dateTruncate already did. @SuppressWarnings("unchecked") sat on getMinArgs() rather than on execute(), where the unchecked Map cast lives. Pre-existing, and in all five files rather than the two reported. Also records the PR and review history in the tracking doc.
|
Cycle 2 addressed in aa83404. Both code points applied, with a correction on the third. 2. Weak assertions - applied, and this was the right catch. The four truncate default-path assertions checked only
3. 1. Changelog - agreed on substance, but there is nowhere to put it. The repository has no On follow-up 5 (two implementations of Verification after these changes: 234 tests green across |
Review: #5629 - explicit
|
parseRoundingMode(args[2]) ran before the propagation check, so an explicitly written null mode was parsed to HALF_UP and the result then discarded. Harmless, but it made the method's own Javadoc untrue: it claimed an explicit null never reaches it. execute() now skips the parse for an explicit null, which propagates immediately after. A mode that is present and not null is still parsed before propagation, so round(null, 2, 'SIDEWAYS') keeps reporting the unusable mode.
|
Cycle 3 addressed in d28f759. 1. Fixed by short-circuiting rather than by softening the comment, so the contract becomes true instead of merely accurate: final RoundingMode mode =
args.length == 3 && !CypherFunctionHelper.isExplicitNull(args, 2) ? parseRoundingMode(args[2]) : RoundingMode.HALF_UP;The ordering guarantee is unaffected - a mode that is present and non-null is still parsed before propagation, so 2. Truncate computes before short-circuiting - declining, and not only on cost grounds. I tried the suggested Skipping This is the difference between the two cases: in 3. Follow-up #1 priority - agreed. Noted for a fast-follow; it is the raw |
Review: #5629 - explicit
|
isExplicitNull(args, 2) was evaluated twice in round(), once to decide whether to parse the mode and once in the propagation guard. One local makes the two branches obviously consistent. Also records the truncate family's silent acceptance of a present, non-null, non-map third argument as follow-up 6, with a note on why it is deliberately left untested rather than pinned.
|
Cycle 4 addressed in f1b2f25. This is the last automated cycle - the branch is handed back to @robfrank from here. 1. Double evaluation of 2. Truncate accepts a present, non-null, non-Map third argument - agreed it is a defect, recorded as follow-up 6. On the suggested test: deliberately not adding one. A test asserting that 3. Agreed on the deferred items. Cycle summary
CI green throughout: Two items left for the maintainer:
|
Review: fix(engine) #5629 - explicit
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5699 +/- ##
============================================
- Coverage 67.49% 66.76% -0.73%
- Complexity 0 1114 +1114
============================================
Files 1770 1771 +1
Lines 150078 150269 +191
Branches 31809 31873 +64
============================================
- Hits 101290 100323 -967
- Misses 35607 36887 +1280
+ Partials 13181 13059 -122 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Closes #5629
The decision
The issue asked to settle whether an explicit
nullin an optional argument position propagates or means "use the default", and explicitly warned against settling it per-function. It also asked for Neo4j's behaviour to be checked first, which the reporter could not verify.Settled in favour of propagation. Three independent lines of evidence:
neo4j/docs-cypher):round()"returnsnullif any of its input parameters arenull";replace(original, search, replace [, limit])"If any argument isnull,nullwill be returned";btrim("hello", null)andltrim("hello", null)both "returnnull".round()is the direct counterpart of the case in the issue. (Neo4j is not uniformly propagate -substringandleft/rightraise on a null length - but in no documented case does it read an explicitnullas "argument omitted". That is the invariant adopted here.)CypherSubstringFunctioncarriesIssue #5193: an explicitly supplied null length propagates null (as Neo4j does), it must not be treated as an omitted argument.Same reasoning, same citation - just never written where the next function would find it.getMinArgs()differs fromgetMaxArgs(): 9 already propagated (the five temporal constructors,point(),ltrim(),rtrim(),vector_create()), 11 silently defaulted, 1 throws by design (range(), [OpenCypher] size() returns null for unsupported argument types instead of raising a type error #5477). Option 2 would have meant changing the ones that were already right.The rule
Stated once, on
CypherFunctionHelper.isExplicitNull(args, position), and called rather than re-decided. That is the mechanism the issue asked for - settling it one function at a time is how the arity declarations drifted in #5484 and hownormalize()andisNormalized()came to disagree in #5602.Changes
Eleven functions adopt it - the five named in the issue plus six the audit found:
normalize/isNormalizednullformattoString()nullroundnullvector.distancenulldate/datetime/localdatetime/time/localtime`` .truncatenullSubstringFunctionnullOmitting the argument still selects the default in every one of them; only the explicitly-written
nullchanges.Two of the five named functions were not where the issue pointed
Both are dual-path cases:
substring()- Cypher resolves it toCypherSubstringFunction, which already propagated (Explicit null length insubstring()is treated as an omitted argument #5193).com.arcadedb.function.text.SubstringFunctionis a second implementation that no factory registers and that still defaulted. Aligned here so the divergence does not surface the day it is wired up; covered by a direct unit test since no query can reach it.vector_distance()- in Cypher this is a grammar rule whose metric is a keyword, sovector_distance(a, b, null)does not parse (the same situation as Neo4j'snormalize()).VectorDistanceFunctionis reached from Cypher asvector.distance(), where the metric is an ordinary expression. That is the path fixed and tested.Ordering is preserved
Propagation is placed after the existing argument checks, not before, so a null in the optional position cannot mask a client error in an earlier one:
round(null, 2, 'SIDEWAYS')still reports the unusable mode, and a bad truncate unit or non-temporal value is still reported. Both are pinned by tests.Test plan
CypherOptionalArgumentNullIssue5629Test(new, 17 tests) - for each of the eleven functions: the explicitnullanswersnull, and omitting the argument still selects the documented default. Confirmed failing before the fix (11 failures), passing after.round()and the truncate family.mvn -pl engine test -Dtest='Cypher*Test,OpenCypher*Test,TextStatelessFunctionsTest,*TemporalFunction*Test,*VectorFunction*Test,SQLFunctionPhase*Test,*StatelessFunction*Test'- 3060 tests, 0 failures.mvn -pl engine testrun as a backstop.Two existing assertions changed
Both encoded the behaviour this issue decided to change; both were found by running the suite, and each leaves what the test was written to prove intact.
CypherNumericFunctionArgumentIssue5484Test.theRoundingModeOfRoundIsNotANumericArgumentassertedround(3.14159, 2, null) == 3.14, now expectsnull. Its point - that the mode position is not rejected as non-numeric - is still pinned by the'FLOOR'and'CEILING'cases.TextStatelessFunctionsTest.formatFunctionNullPatternasserted a null pattern answerstoString(), now expectsnull, and gained an assertion that omitting the pattern still answers the ISO string.Follow-ups (not in this change, deliberately)
*.truncatefamily raises raw JDK exceptions for client mistakes:NullPointerExceptionon a null unit (args[0].toString()is unguarded;TrimFunctionhas the same exposure on its mode), andIllegalArgumentException: Unknown truncation unitfromTemporalUtil. Both surface as HTTP 500 for the caller's error - same class as [OpenCypher] abs() returns HTTP 500 for invalid argument instead of client-facing error #5484. The new test asserts only the message for the unknown-unit case, so it does not encode the wrong exception class as correct.trim(mode, trimChar, source)defaults a nulltrimCharto whitespace, disagreeing withltrim/rtrimbeside it and with Neo4j'sbtrim("hello", null). Its 1-or-3 arity means the argument is not trailing, so it is a different shape.range()throws on a null step rather than propagating, by explicit decision in [OpenCypher] size() returns null for unsupported argument types instead of raising a type error #5477 - worth confirming that is still intended now the convention is written down.substring()diverges from Neo4j here: Neo4j raises for a nullstartorlength, ArcadeDB propagates for both. Propagating is internally consistent (its nullstartalready answerednull, and Explicit null length insubstring()is treated as an omitted argument #5193 chose this for the Cypher-facing implementation), but the divergence is worth a decision.com.arcadedb.function.text.SubstringFunctionis registered by no factory - either wire it up or delete it, rather than keeping two implementations of one function.Full reasoning and the complete audit table are in
docs/5629-cypher-explicit-null-optional-argument.md.🤖 Generated with Claude Code