DDIR: ordering, source shapes and scalar operations for SNB reads - #863
Merged
Conversation
Keep unary eligibility exhaustive, correct the has_shape indentation, and document empty-list inference and the structural floating representation. Disclose fatal ascription assertions and the cost of lexicographic list ranking without changing either behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DDIR: ordering, source shapes and scalar operations for the SNB reads
Base:
master-next. Part one of the benchmark foundation; the runnablebenchmark is the separate, stacked PR #864.
This is the engine-prerequisite part of the LDBC work. The runnable benchmark
follows separately.
MinandCollectagree with DDIR's observable signed-integerand lexicographic list ordering. Physical arrangement-key ordering is not
changed, nor is Corgi's general structural ordering.
inactive SUM lanes cannot reveal their full shape from rows. These are
execution-time contracts, enforced by assertions at the boundary; they are
not transactional feed validation or a new static type system. A mismatched
row can panic a dataflow worker on either backend and take down the shared
server, disconnecting all clients. A feed acknowledgement does not certify
its shape. There is no per-program failure isolation; this foundation assumes
trusted programs and shape-correct data. This assertion adds a fatal failure
path to the Vec backend; Corgi's transcoding could already panic on misfit rows.
append, value-only projection for collect, declared empty-list constructors,
and explicit floating conversion/arithmetic. Pin Corgi to the merged public
division revision
be00398; no local dependency override is required.The list-ordering adapter is not performance-neutral. The reviewer measured
100,000 rows, comparing the old length-first sort against rank construction
plus sorting the ranks:
These are reviewer-supplied measurements, not independently reproduced here
and not whole-query slowdown ratios.
Min/Collectuse this ranker forlist-valued subcolumns, including nested lists and strings represented as lists.
Non-list values do not run the list ranker. It eagerly ranks every element,
including unused tails, then scans all lists and allocates scratch per prefix
round until prefixes distinguish values or reach their ends. The fixed-length
lists of scalar elements measured here already have the same length-first and
lexicographic order, so these cases expose implementation overhead rather than
an unavoidable cost of changing the answer. Ranking optimization and server
admission/isolation are separate follow-ups.
The floating representation uses an existing one-variant SUM carrying encoded
f64 bits, with explicit operators and total ordering. There is no new nominal
runtime type: a user's single-variant integer newtype also passes the structural
checks for
fnegand binary floating operators, and its payload is interpretedas encoded f64 bits. This is a type-checking limitation. The representation also
differs from LDBC's Float32 API.
Integer division returns zero on a zero divisor and wraps
i64::MIN / -1,matching the selected Corgi operation.
A declared constructor can supply the element shape for empty
list(), butCorgi's shape inference does not propagate between
appendarguments:append(list(), xs)is rejected even whenxshas a known element shape.The branch is replayed onto
master-nextat660ff8da. The only conflict wasthe dependency pin, resolved to the merged revision. Benchmark definitions,
data, profiling hooks and optimizer experiments are not in this diff.
Before the review cleanup, 118 Rust library/integration tests passed (four
existing heavy tests remained ignored). The unchanged downstream LDBC fixture passed the local
CI command matrix on both backends: four-query panel at one/four workers,
all-41 mixed at one/four workers, and all-41 maintained at four workers.
These are local macOS checks, not a claim that hosted CI ran. Full-suite checks
use the same logical-plan Python evaluator, not an independent LDBC oracle.
An optimized server build and the same downstream fixture matrix also pass
with
CARGO_PROFILE_RELEASE_DEBUG=0(opt-level 3, LTO, one codegen unit retained).This does not claim that the ignored heavy Rust checks or all hosted CI jobs ran.
Review cleanup (
5d08c7b2) makes unary eligibility an exhaustive match, fixesthe
has_shapeindentation and documents the limitations above; it changes noruntime semantics and adds no tests. Fresh checks pass: all 53 library tests,
all eight server integration tests, the benchmark's Python checks, and all 41
reads on both backends with four workers in mixed and maintained modes using
the rebuilt optimized server and tiny witness. The reviewer's ranking timings
and fuzz campaigns were not rerun.
The separately reproduced upstream chunk merge-horizon issue is fixed in #862.
This PR neither introduces nor includes that fix; small-fixture passes must
not be taken as evidence that the issue is resolved.
Squash merging is fine. Preserve this branch's old tip before merging, then
replay only the benchmark branch's own commits onto the actual squash merge.