Skip to content

TUR-298: Cypher: implement coll.sortMulti - #1455

Open
sulaimansuhas wants to merge 1 commit into
mainfrom
suhasnarreddy/tur-298-implement-apoccollsortmulti
Open

sulaimansuhas wants to merge 1 commit into
mainfrom
suhasnarreddy/tur-298-implement-apoccollsortmulti

Conversation

@sulaimansuhas

Copy link
Copy Markdown
Contributor

Implement coll.sortMulti.

Stacked on #1452 (bot stack main): review and merge #1452 first.

@sulaimansuhas

Copy link
Copy Markdown
Contributor Author

Note

AI-generated summary.

Summary

Architecture of coll.sortMulti in the v3 engine: FunctionDecls and codegen emit db.sort_multi, lowerSortMulti builds nl.sort_multi, runSortMulti sorts each row and writes the result through ListBuffer::gather

coll.sortMulti(maps, fields, limit, skip) (APOC's apoc.coll.sortMulti) now runs end to end on v3. It follows the list_slice shape: one db op, one nl op, one executor step.

 FunctionDecls            + coll.sortMulti: 1-4 args (list, list, int, int), each also Any
 DBProgramGenerator       + translateSortMulti -> db.sort_multi(list, fields?, limit?, skip?)
 DBLowering
+  rowAlignOptionalChunks   lay the present operands out over one row count
+  lowerSortMulti           -> nl.sort_multi, result element type = input's
~  lowerListSlice           same IR, now uses rowAlignOptionalChunks + listOfSameElementsChunkType
 NLTranslator             + translateSortMulti -> NLSortMultiData
 NLExecutor               + runSortMulti
 ListBuffer               + gather(span<CellView>); reverse() now shares writeElements

Sorting one row's list: null check, field parse, key extraction, stable sort, write of the kept slice into the list buffer

Per row, runSortMulti does this:

if list, fields, limit or skip is null -> null cell
skip < 0                               -> raise
'^key' sorts ascending, 'key' descending; a non-string field raises
for each element: not a map -> raise; read each key (missing or null = null)
no fields -> elements->slice(skip, limit)          # a view, no copy
else stable_sort an index permutation on the keys  # nulls first asc, last desc
     -> listBuffer.gather(maps[skip .. skip+limit])  # limit < 0 keeps all

Values of different types compare as ORDER BY compares them ('a' < true < 1 < 2.5). The field list is parsed again only when its storage changes, so a constant field list is parsed once per chunk.

Evidence

New test test_query_ir_coll_sort_multi (test/query/ir/list/CollSortMultiTest.cpp), 19 cases. Before this PR, coll.sortMulti is not a known function, so every case fails in the analyzer. Examples of the expected answers:

coll.sortMulti([{name:'b'},{name:'c'},{name:'a'}], ['^name'])  -> {name: a}, {name: b}, {name: c}
coll.sortMulti([... {name:'are', age:21}, {name:'are', age:50} ...], ['^name','age'])
                                                                -> {age: 50, name: are}, {age: 21, name: are}, ...
coll.sortMulti([{a:3},{a:1},{a:2},{a:4}], ['^a'], 2, 1)          -> {a: 2}, {a: 3}
coll.sortMulti([{a:2},{b:1},{a:null},{a:1}], ['^a'])             -> {b: 1}, {a: null}, {a: 1}, {a: 2}
coll.sortMulti([{a:1}], null)                                   -> null
UNWIND [['^a',1],['a',0]] AS spec
  RETURN coll.sortMulti([{a:1},{a:3},{a:2}], [spec[0]], 2, spec[1])
                                                                -> {a: 2}, {a: 3} | {a: 3}, {a: 2}
coll.sortMulti([{a:1}, 2], ['^a'])        -> raises "coll.sortMulti sorts a list of maps"
coll.sortMulti([{a:1}], 'a')              -> rejected: "Invalid arguments for function 'coll.sortMulti'"

The other cases cover stable ties, an empty field list, limit 0 / -1, a stored list property, a collect()ed list, UNWIND of the result, the upper-case name and a negative skip.

I did not build or run the tests for this comment.

Merge Danger

Door: two-way

The change adds a new function, two new ops and a new executor step. It writes nothing to disk and changes no wire format.

Blast Radius: small

Two existing paths change shape: lowerListSlice now goes through rowAlignOptionalChunks, and ListBuffer::reverse through the new writeElements template. Both must give the same output as before, so a regression would show in the list-slice and reverse() tests. The PR is stacked on #1452.

@sulaimansuhas
sulaimansuhas force-pushed the suhasnarreddy/tur-298-implement-apoccollsortmulti branch 2 times, most recently from 2cf3fea to f0baac9 Compare November 11, 2026 10:26
@sulaimansuhas
sulaimansuhas marked this pull request as ready for review November 11, 2026 10:28
@sulaimansuhas
sulaimansuhas requested a review from rjb32 as a code owner November 11, 2026 10:28
Base automatically changed from suhasnarreddy/tur-134-mlir-unwind-a-typed-list-of-datetimes to main November 11, 2026 12:54
@sulaimansuhas
sulaimansuhas force-pushed the suhasnarreddy/tur-298-implement-apoccollsortmulti branch 2 times, most recently from 683e7d8 to 544f8fd Compare November 11, 2026 14:01
@rjb32

rjb32 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

DISABLED_sortsTheLegsWithSortMulti in test/query/ir/journey/JourneyParetoTest.cpp:176 is the test TUR-298 was waiting on, and it is still disabled. It still calls apoc.coll.sortMulti and still says the function does not exist. Please enable it in this PR, as f1a0fc0 did for the coll.indexOf journey test:

-// Disabled: apoc.coll.sortMulti does not exist (TUR-298).
-TEST_F(JourneyParetoTest, DISABLED_sortsTheLegsWithSortMulti) {
+TEST_F(JourneyParetoTest, sortsTheLegsWithSortMulti) {
 ...
-               "RETURN [leg IN apoc.coll.sortMulti(OTTlist, ['nlc', '^arrival', 'departure']) | leg.service]",
+               "RETURN [leg IN coll.sortMulti(OTTlist, ['nlc', '^arrival', 'departure']) | leg.service]",

@rjb32

rjb32 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Other items to fix in this PR:

  1. db.sort_multi is MayRaise with no ConditionallySpeculatable (DBOps.td:690). Lowering then takes a call over constants through raisesOverConstants and runs it once per row. MATCH (n:Person) RETURN coll.sortMulti([{a: 2}, {a: 1}], ['^a']) sorts the literal list once per person instead of once. Implement getSpeculatability to answer speculatable when the operand types rule out every raise, as ListSlice::getSpeculatability does.

  2. Neither db.sort_multi nor nl.sort_multi has a verifier (NLOps.td:522). A badly typed operand is only caught in NLTranslator, with an error naming list slice or nl.range. runSortMulti casts the result to ColumnOptVector<ListView> with nothing in the IR to guarantee that type.

  3. setReturnsItsArgumentShape(true) is set on every overload, including the ones whose first argument is an Any cell (FunctionDecls.cpp:1001). tail's cell overload leaves it off on purpose (FunctionDecls.cpp:619). Set it only where the first argument is a List.

  4. The comments on runSortMulti (NLExecutor.h:463) and nl.sort_multi say each row is a new list in the query's list buffer. The sorted list lives in the step's buffer, which is cleared every run. With no fields, the result is a slice of the input list.

  5. Two consecutive ifs that both leave the loop body, with no else (NLExecutor.cpp:9859): if (readsANull) { ...; continue; } then if (*skipped < 0) { throw ...; }. Make it an else if.

  6. CollSortMultiTest has no case for a list held in an Any cell (UNWIND [[{a: 2}, {a: 1}], 1] AS xs RETURN coll.sortMulti(xs, ['a']), which sorts the first row and raises on the second). It also has none for more rows than one chunk holds.

This branch has not been deployed

No deployments
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.

2 participants