[VARKA] A year that does not compute the month - #80
Merged
Conversation
PLAN_MILESTONE_4.md section 2.18 predicted this task and task 32 step B were in tension: once the prefix is shared it computes the March month for the month, day-of-month and quarter tails regardless, so "whichever of the two lands second inherits the smaller half of the win" - one op instead of five. Step B1 landed first. This plan does not take the smaller half. The month step is dead work exactly when no consumer of that prefix reads marchMonth, and planSlots already walks every consumer of a body, so the step becomes conditional on a per-body consumer set: a year-only loop method elides it (the full five-op win, on the TPC-H q7/q8/q9 shape), year(d), month(d) in one method correctly keeps it, and dayofyear and trunc(d, 'YEAR') - which test doy >= 306 themselves - get the same elision for free. The decision is read from the plan-time set over all consumers, never from whichever sibling emits first, so it is order- independent under sharing; with sharing off it is per node, since equal keys share no locals there. The identity itself (mp >= 10 <=> doy >= 306) is proved in three lines in section 2 and then asserted over all 366 values through the shipped constants, which is what "recorded as an integer identity rather than an approximation" ought to mean. The scalar twin's year line moves with it so the model keeps mirroring the emitted arithmetic; VarkaChronoSuite's exhaustive sweep is unchanged and must stay green. Registered before measuring: a year-only loop body drops 45 -> 41 IntVector ops (deterministic, asserted); by minimums the timing lands at 1.00-1.06x at AVX-512 and inside noise at 128-bit, and inside noise is an accepted outcome - the default ships on the provably-dead-work argument, the number is recorded because the repo's rule says a claim traces to a committed file. The unshared HugeMethodLimit crossing moves a third time, 19 -> 20 (every Year node's own prefix shrinks under unshared), shared stays at 44; the ladder, PLAN_TASK_32.md 7.1 and the debt register are updated rather than the number absorbed. Also touches PLAN_TASK_35.md 7.5 (tailReadsMarchMonth becomes an edit site for TruncDate) and the milestone's row 48 and 2.18 update note. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
The year tail needs one bit out of the March-based month: whether the March year has turned January, which is `mp >= 10`. Since `mp = (5 * doy + 2) / 153` is exact over the whole domain the prefix leaves, that bit is `doy >= 306` - an integer identity, not an approximation: mp >= 10 <=> 5 * doy + 2 >= 1530 <=> doy >= 305.6 <=> doy >= 306 `VarkaChrono.MARCH_TO_JANUARY_DAYS` names the threshold and its javadoc carries that derivation; `VarkaChronoSuite` asserts it over all 366 values of the domain through the shipped constants, along with the exactness of the month magic the identity rests on, because 366 cases are cheaper to run than three lines of algebra are to trust. `emitChronoYear` therefore takes the day of year instead of the March month and gets its mask from a new `emitJanuaryMaskFromDayOfYear`; `emitJanuaryMask` stays for `emitChronoMonth`, which has the month in hand anyway. `rem` is intact at both call sites - each calls `emitChronoYear` first thing after the prefix. The scalar twin's year line moves with it so `VarkaChrono` keeps mirroring the emitted arithmetic, which is what makes a disagreement between the two an emission bug rather than an arithmetic one. No elision yet: the prefix still computes the month unconditionally, so the only byte that moves is `sipush 306` where `bipush 10` fit, one per year tail. That is why "sharing the prefix leaves every loop method byte for byte as it was" and the HugeMethodLimit ladder at 44 both stay green here, and why the elision behind `VarkaEmitOptions.elideChronoMonth` is a commit of its own. Both exhaustive sweeps re-run green over all 16777216 covered days: the scalar model against `LocalDate`, and the emitted calendar kernel against it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
The civil-from-days prefix ended with the March-month step unconditionally, whether or not any tail that followed read it. It is dead work exactly when no consumer reads `marchMonth`, and after step 1 the year tail is such a consumer: it takes the January turn off the day of year instead. So the step becomes conditional, behind `VarkaEmitOptions.elideChronoMonth` (default on). Two pieces. `tailReadsMarchMonth` is an exhaustive switch over the same family `chronoChild` covers, so a future calendar node is a compile error rather than a silent "yes" that costs five ops or a silent "no" that reads an uninitialised local - and the latter is rejected by the verifier at class load rather than read as garbage, which is the right failure mode for a mistake of that kind. `planFragmentsReadingMonth` then collects, for the lane group about to be emitted, the fragments some tail in it reads the month out of, and the prefix emits its last five instructions only for those. The scope is the correction this step made to the plan. PLAN_TASK_48.md 3.2 said the set was per body, on the belief that `Slots` is planned per body method. It is not: `planSlots` receives the kernel's whole output list, and it is the group threaded into `emitBody`/`emitLaneGroup` that narrows a loop method to its own outputs - so a body-scoped set kept the month step in a `year(d)` loop method merely because `month(d)` was another output, losing the elision this task exists for. "sharing the prefix leaves every loop method byte for byte as it was" caught it on the first run. The set is now computed per lane group, beside the `emittedFragments.clear()` that has exactly that scope, which is also what makes it sound: a fragment is re-earned in each lane group, so what has to hold is that every reader of t[5] in a group is preceded by a write of it in that group. Reading the set rather than the node being emitted is what keeps the decision order-independent under sharing. Measured, not assumed: a year-only loop body drops from 43 to 39 `IntVector` ops, counted off the class file through a new `VarkaEmitterTestSupport.invocationCount`. The registered prediction said 45 to 41 - the four-op delta is exact, the absolute pair was two low because the prediction counted two `VectorMask` invocations that this counter does not. Recorded in the plan rather than quietly corrected. The epilogue's unshared `HugeMethodLimit` crossing moves from 19 outputs to 20 (7953 bytes at 19, 8386 at 20); shared stays at 44, gaining only the byte a year tail's `sipush 306` costs over a `bipush 10`. That is the third move of that number for the third unrelated reason, so the full ladder is re-measured and appended to PLAN_TASK_32.md 7.1 beside the other two rather than overwriting them, and the milestone's debt register says so. Tests: the op count above; order independence, where `Seq(Year, Month)` and `Seq(Month, Year)` under sharing both keep the step in the epilogue and both elide in exactly one loop method; the unshared per-node case, where the year's own prefix elides and the month's does not; and the exhaustive sweep extended to a year-only kernel under both sharing modes and both switch positions, since the four-field shape keeps the step under sharing and so never sweeps an elided prefix there. All Varka suites green in catalyst and sql at both vector widths, sweeps included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
Two adjacent cases in the parity benchmark's year section - the shipped kernel
and the same kernel with the month step put back - null-free and mixed-null
each, interleaved so one regeneration runs both sides back to back under the
same JIT and thermal state. Five regenerations at each width on an idle
machine, compared by minimums.
By minimums, in M rows/s:
elided kept ratio
AVX-512 null-free 2201.9 2181.7 1.01x
AVX-512 mixed nulls 2161.7 2145.2 1.01x
128-bit null-free 746.3 742.7 1.00x
128-bit mixed nulls 751.9 750.1 1.00x
Inside noise, and the sign is not stable - in two of five AVX-512 runs and two
of five 128-bit runs the kept side won the mixed pair. That is the outcome
section 6.0 registered before measuring: four ops off a 43-op body at a 9 ms
best time is under half a millisecond, below what the file resolves. The
default ships on the step being provably dead work where it is elided and on
the exhaustive sweep, with the number recorded because the repo's rule is that
a performance claim traces to a committed file.
The regeneration then moved the calendar rows far more than that A/B can
account for - `year, null-free` 1823.4 to 2166.5, `year+month, separate` 913.7
to 1042.8, `year(d1), year(d2)` 891.4 to 1040.9 - and none of it is this task.
The in-run control says so first: `per-row LocalDate year`, which no Varka
change touches, reads 481.1-481.6 across all five runs against a committed
479.4, so the machine had not drifted and the kernel rows moved for a real
reason. The A/B pair says this task's own share is 1.01x. And the file was last
regenerated at 06d9664 (task 32 step B1), after which exactly two commits
touched the emitter: 71ebc64, task 51's removal of the per-extraction range
guard, and cb176a0, task 38's column offsets, which no calendar kernel
runs. By elimination it is task 51's, and the shape fits - the guard was paid
once per calendar node, so a one-field kernel gains a fifth while the
four-field shared kernel, paying it once for four tails, does not move at all
(799.8 to 797.7).
So task 51 shipped a ~19% win to every single-field calendar kernel and never
regenerated the parity file. Recorded three ways rather than absorbed: in
PLAN_TASK_48.md 9.2 with the attribution argument, in SKILLS.md as the general
lesson (regenerate in the task that changed the bytes, and keep an in-run
control and an in-run A/B so a false 21% cannot be written down), and in
PLAN_TASK_45.md, whose "today" baseline column was stale in every calendar row
and now says so.
Predictions scored in PLAN_TASK_48.md 9.3: 2, 3, 4 and 5 held; 1 missed in the
absolute only (43 to 39 ops, not 45 to 41 - the delta was exact); 6 carries
forward to whichever of this and PR apache#64 merges second. Milestone row 48 marked
DONE.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
Velox was read alongside Varka's calendar lowering during task 48, and its scalar decomposition (velox/type/FastDate.h) turns out not to follow Hinnant's civil_from_days - the algorithm task 26 lowered - but Neri-Schneider 2022. The paper's shape is better in two places and exactly one of them reaches an IntVector lane, so the plan takes that one and says clearly why it leaves the other. What it takes is the month block. Neri-Schneider gets the month index and the day of month from a single affine numerator, `num = 2141 * doy + 197913`, with the month as `num >>> 16` and the day of month out of `num & 0xFFFF`, where Varka computes the month with a magic multiply and then recovers the day of month by running a second magic multiply forwards (`emitMonthStart`) and subtracting. Verified over the whole domain the prefix produces rather than taken on the paper's word: `num >>> 16` minus 3 equals `(5 * doy + 2) / 153` and the remainder form equals `doy - monthStart(mp)` at all 366 values, zero mismatches; the one division it needs, `/2141` over [0, 65535], has an exact magic inside a lane, `(x * 31345) >>> 26`, checked at all 65536 values with a maximum product of 2054194575. The inverse direction's month start is a shift too, `(979 * m3 - 2919) >>> 5`, equal to Varka's form at all twelve months. Registered op counts: -2 on `month`, -4 on `dayofmonth`, -2 on `quarter`, -3 on `trunc(d, 'MONTH')`, -1 per `emitMonthStart` call, and `year` unchanged. What it does not take is the paper's headline - the correction-free century and year steps - and section 2.2 is a hard boundary rather than an omission. `century = (4 * shiftedDay + 3) / 146097` has a dividend up to 2^26, at which the largest multiplier whose product still fits a signed 32-bit lane is 32, while an exact magic needs about 29 with a 1% divisor error; searched, not argued, and no (M, k) is exact over that range inside a lane. The year step needs the high half of a 64-bit product, and `VectorOperators` still has no multiply-high on any lane type - the fact task 26's admission check turned on. Both wait for task 49's int64 lanes. Two things recorded before they are discovered. The month axis moves to Neri-Schneider's March = 3, because that is what removes the add in front of the reported month, so `MARCH_YEAR_JANUARY` becomes 13 and every reader of the index moves at once - mitigated by renaming the local so a missed call site fails to compile rather than computing a month three off. And task 48's elision shrinks from four ops to two once the month step is a two-op numerator; PLAN_TASK_48.md 9.1 now says so rather than being contradicted later. Licensing: Velox's file carries an MIT header for the code it adapts. The identities are from the published paper and free to implement, and this plan re-derives every constant against Varka's own domain rather than importing one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
year that does not compute the monthyear that does not compute the month
Section 2.1 justified `a' = 2141`, `b' = 197913` at `k = 16` by an exhaustive check over Varka's domain. The paper derives them and states their range itself - Example 10 and Equation (20), valid for all N_Y in [0, 734[, which contains Varka's [0, 365] - and that is the stronger citation, because it covers the reasoning rather than only the values. Both agree. Found while transcribing the paper into `sql/varka/papers/`, which the section now points at, with the warning that the theorem statements are the part the transcription loses and should be read from the PDF. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
This was referenced Sep 2, 2026
MaxGekk
added a commit
that referenced
this pull request
Sep 2, 2026
### What changes were proposed in this pull request? Adds `add_months(d, 13)` to `VarkaEmitterParityBenchmark`, null-free and mixed-null, and commits the baseline. `add_months` is the widest calendar node the emitter has — it decomposes a date, does the month arithmetic and recomposes, all inside one loop method — and nothing in the parity benchmark measured it. Every statement about its cost has therefore been an argument rather than a number, including the one the follow-up PR is about to make about the leap flag inside it. This lands the baseline first, against unchanged code, so that the improvement arrives as a diff in this file rather than as a first appearance. The two cases need their own chunk driver rather than the shared `chunked`, because the month count is a scalar argument; the walk itself is the same 4096-row one the rest of the year section uses. **The baseline**, minimum of three regenerations on an idle machine: | case | Best (ms) | Rate (M rows/s) | |---|---|---| | `add_months(d, 13), null-free` | 36 | 563.9 | | `add_months(d, 13), mixed nulls` | 36 | 557.7 | Worth saying now that the number exists: **this is the weakest calendar node in the file.** At 563.9 M rows/s it is 1.17x the per-row `LocalDate` path the row engine uses, where `year` is about 4.5x it. So `add_months` is where the emitted calendar lowering has the least margin over the code it replaces, and the first place to look when this family is optimised again. ### Why are the changes needed? Because the repo's own rule is that a performance claim traces to a committed benchmark file, and this node had no row at all. A benchmark introduced by the same PR that improves the code commits only the improved number, and the gain then exists nowhere in the history. ### Does this PR introduce _any_ user-facing change? No. A test-scope benchmark case and its results file; no `main` code. ### How was this patch tested? Three regenerations on an idle machine, compared by minimums, with the spread within the three under 1%. **This regeneration also picks up a win that is not this PR's, and that is worth reading before the diff alarms anyone.** Every single-field calendar row moves — `year, null-free` from 1823.4 to about 2170 M rows/s — because the file had not been regenerated since task 32 step B1 (`06d96642707`), and task 51's removal of the per-extraction range guard (`71ebc645605`) landed after it without one. Exactly two commits touched the emitter in between, the other being task 38's column offsets, which no calendar kernel runs, so by elimination it is task 51's; and the shape fits, since the guard was paid once per calendar node. The check that it is not machine drift is in the file itself: the in-run control `per-row LocalDate year`, which no Varka change touches, reads 481.2–481.8 across all three runs against the committed 479.4. PR #80 regenerates this same file for the same reason and records the argument in `PLAN_TASK_48.md` §9.2. Whichever of the two merges second should rebase and regenerate rather than merge this file textually — a benchmark file is a measurement, not a text. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5)
MaxGekk
added a commit
that referenced
this pull request
Sep 2, 2026
…rka/papers (#81) ### What changes were proposed in this pull request? Adds `sql/varka/papers/`, holding machine transcriptions of the third-party papers this work reads closely enough that quoting them from memory would be a mistake, and starts it with one: Neri and Schneider, "Euclidean affine functions and their application to calendar algorithms" (*Software: Practice and Experience* 53(4), 2023, [10.1002/spe.3172](https://doi.org/10.1002/spe.3172), open-access deposit [hal-04346335](https://hal.science/hal-04346335v1)). `PLAN_TASK_53.md` — the Neri-Schneider month block, planned in #80 — rests on constants and validity ranges out of that paper, and a plan that quotes them from memory is how a wrong magic constant ships quietly. Also included: `papers/README.md` stating what these files are and are not, a paragraph in `sql/varka/AGENTS.md` so the rules sit where the directory is, and a `SKILLS.md` section generalising what the conversion turned up. **The conversion is deliberately mechanical, and that is the substance of this PR.** A plain `pdftotext` is useless for this paper: the text layer flattens every superscript and subscript, so `2^16` becomes `216` and `N_C` becomes `NC`, which destroys nearly every formula in it. The transcription is therefore rebuilt from `pdftotext -bbox-layout` word geometry — a glyph smaller than its line's body and off its baseline is a superscript or a subscript — which recovers 1408 of them across 35 pages. Seven more the PDF had already merged into single tokens were restored by name, guarded so that "New York, NY, USA" in the bibliography is left alone. Section titles, which the PDF sets in letter-spaced small capitals that no text layer recovers as words, were restored by hand. What that loses is structure: tall delimiters (set-builder braces, large parentheses, fraction bars) are separate glyphs on their own lines, so the theorem statements in sections 3, 7, 13 and 14 come through fragmented, and figures and tables are gone. The linear algorithm formulas — Algorithms 1-6, which is what the paper is here for — do not use tall delimiters and are intact. **`marker-pdf` was evaluated against exactly that gap, and rejected.** On five pages it does fix it: eight displayed formulas as correct LaTeX with their braces intact, plus 95 table rows including the assembly listings the geometric pass loses entirely. On those same five pages it also silently corrupted four things: | the paper | Marker | |---|---| | `∀n ∈ [0, U[` (Theorem 3) | `[0, U]` | | `∀N_Y ∈ [0, 734[` (Equation 20) | `[0, 734]` | | `∀n ∈ [0, 10441974239[` | `[0, 1044197429]` | | the glyphs for ε and `>` in prose | replacement characters, five lines each | Three half-open intervals silently closed, and one of them lost a digit from an eleven-digit bound — a validity range for a strength reduction, which is precisely the kind of number that would be copied into an admission check. Every one of them reads as plausible. That is what an LLM extraction pipeline does when it cannot read something: it produces something reasonable rather than a gap, and the reader cannot tell which is which. The general rule, in `SKILLS.md`: **for anything a plan will quote as a number, prefer the extractor whose failure mode is a visible hole** — then say in the file what the hole is, and point at the PDF for the parts that did not survive. ### Why are the changes needed? Task 53 and the background for task 26's admission check both cite this paper's constants. Having the argument in the repository means a reviewer can check a magic number against the reasoning behind it without hunting down a PDF, and means the citation cannot quietly drift from what the paper says. ### Does this PR introduce _any_ user-facing change? No. Documentation only; no code changes. ### How was this patch tested? There is nothing to run, so the checks are on the artifact itself. The script's recovery was counted rather than eyeballed (1408 scripts marked; seven bare tokens left, all traced by hand); the prose was diffed for words the column-rendering could have run together (none outside the discarded Wiley watermark); and the four numeric corruptions above were found by diffing multi-digit numbers between Marker's output and the PDF's own text layer, then confirmed one by one against `pdftotext` on the relevant page. One cross-check worth recording: the paper's Example 10 gives `a' = 2141`, `b' = 197913` at `k = 16`, valid for `N_Y ∈ [0, 734[`, which is the derivation `PLAN_TASK_53.md` §2.1 rests on. Varka's domain is `[0, 365]`, inside it. That agrees with the independent exhaustive check the plan already carried, which is the outcome one wants from a second source. ### Two things for the reviewer **Licensing.** This is a third-party work from the paper's HAL open-access deposit, under its own authors and terms. `papers/README.md` states that nothing in the directory is Apache-licensed, compiled, or shipped, and that if a paper's terms do not permit a copy being kept, the reading notes and the citation are what this project actually owns and the transcription should go. That is a judgement worth making deliberately rather than by merging. **Non-ASCII.** The transcription carries non-ASCII on 862 lines — the paper's own notation, U+2215 for the quotient of Euclidean division and so on. This is intended: the repo's ASCII rule is about code and comments, and a paper transcription is neither. The other three files are pure ASCII. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5)
# Conflicts: # sql/catalyst/benchmarks/VarkaEmitterParityBenchmark-jdk25-results.txt # sql/catalyst/src/test/scala/org/apache/spark/sql/VarkaEmitterParityBenchmark.scala
Merging master brought in the add_months benchmark PR, which regenerates this same file. Merging the two versions textually would commit a file that is half one measurement and half another, so the file was regenerated once on the merged code instead - one run, containing both that PR's add_months rows and this one's year A/B rows, from a single process. The fresh file agrees with what section 9.1 recorded from five regenerations on the pre-merge code: the year A/B reads 2190.7 against 2175.3 null-free and 2074.9 against 2068.2 mixed, that is 1.01x and 1.00x, the same inside-noise result. add_months is 560.5/561.9 against the 563.9/557.7 the benchmark PR committed, unmoved as expected since this task does not touch it. The in-run control, per-row LocalDate year, reads 481.5. PLAN_TASK_48.md 9.2 gains a note saying the benchmark PR landed first and is therefore where task 51's unregenerated win appears in the history. The section is otherwise left as written: it is the record of how the discrepancy was found and attributed, and only which commit carries the numbers has changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
MaxGekk
added a commit
that referenced
this pull request
Sep 2, 2026
…lendar arithmetic (#84) ### What changes were proposed in this pull request? Adds `VarkaVectorApiProbeBenchmark`, which prices three ways to get a `year` from an epoch day on lanes, and answers a design question milestone 4 has been carrying as an assumption. Impala reads `year` out of a day-indexed lookup table covering 1950-2049 and computes it only outside that window, on the argument that the memory is warm. `PLAN_MILESTONE_4.md` item 9 leans the other way — it assumes a gather is the slow primitive — and nothing in the repo had measured either. This measures both, twice: once in the shape the question is usually asked in, and once in the shape Varka actually runs. **Unfused — one field written to an `int[]`**, 20M dates at AVX-512: | | whole 100-year table (143 KB) | seven-year span (~10 KB, TPC-H shaped) | |---|---|---| | `IntVector` gather | 3573.9 M rows/s | 3728.2 | | `IntVector` arithmetic | 2379.8 | 2368.3 | | scalar `int[]` loop | 3766.3 | **4630.0** | Two expectations died. A gather is **not** automatically slower than forty lane ops on this hardware, even when the table overflows L1; and the **scalar** loop is fastest of the three, because the Vector API takes a gather's index map as an `int[]`, so the index vector is stored and read back — a spill that belongs to the API, not the machine. **Fused — `year(d) = 1998`, counted, where the vector paths never leave a register:** | | M rows/s | | |---|---|---| | gathered from the table, compared in lanes | **3999.8** | 2.8x | | arithmetic in lanes, compared in lanes | 1453.0 | 1.0x | | scalar lookup inside a vector kernel (spill, scalar, reload) | 1446.9 | 1.0x | | scalar loop end to end, no lanes anywhere | 848.1 | 0.58x | **The ranking inverts, and that is the point of the PR.** The scalar loop that led the unfused table by 1.95x loses the fused one by 1.7x — 4630.0 down to 848.1 — because once the result is compared and counted, a per-row loop cannot keep up with lanes doing the same work sixteen at a time. And the hybrid an emitter would actually have to produce, spilling the lane group to an array and reloading it, is a **wash** with the arithmetic: the spill costs exactly what the lookup saves. So *emitting scalar ops with a lookup table buys nothing inside a fused kernel* — which is the question this was run to settle, and it settles it in the negative without anyone having to build it first. What does win is the gather, by 2.8x, because it replaces forty lane ops while the compare and the count stay in registers. That lowering is blocked by one thing: `IntVector`'s index-map overload exists on `fromArray` and not on `fromMemorySegment`, and every Varka input is an off-heap Arrow buffer. Enumerated rather than assumed — the whole `from*`/`into*` surface is `fromArray(species, int[], int, int[], int)` and `fromMemorySegment(species, MemorySegment, long, ByteOrder)`, with no third form. So the API gap is now a **measured 2.8x on the corpus shape** rather than a remark in a design note, and item 9's paragraph says so, since that is the one case where the dictionary genuinely is on-heap and small. ### Why are the changes needed? Item 9's design input asserted what a gather costs without a number, and the repo's own rule is that a performance claim traces to a committed benchmark file. Both halves of that assertion turn out to be wrong in ways that change what the item should plan. ### Does this PR introduce _any_ user-facing change? No. A test-scope benchmark, its results file, and two documents; no `main` code. ### How was this patch tested? Every timed path is checked against `java.time` over every input **before** timing — the unfused ones row by row, the fused ones against `java.time`'s own count — because a benchmark that measures a wrong answer is worse than none, and both kinds are easy to get subtly wrong: the gather by an off-by-one on the table base, the arithmetic by a transposed carry. The declared row count is 20 passes over a 1M-row buffer rather than a single pass, because one pass is under a millisecond and the results file's `Best Time` column cannot resolve that — the same shape `VarkaEmitterParityBenchmark` uses, and the reason it uses it. Two things recorded rather than smoothed over. All three vector paths end in `VectorMask.trueCount()`, and the arithmetic drops from 2368.3 unfused to 1453.0 fused, which is more than a compare should cost; whatever that is, the gather and the arithmetic pay it equally so the 2.8x between them stands, and the scalar path does not pay it and loses regardless. And the arithmetic mirrors master's lowering including the March-month step that #80 removes from the year tail — when that lands, the arithmetic side gains four lane ops and the ratio moves in its favour by that much. Still unmeasured, and said so in `SKILLS.md`: writing to a `MemorySegment` rather than an `int[]`, null handling, and the batch-level fallback a 1950-2049 table needs for the `0001..9999` range SQL allows. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5)
# Conflicts: # sql/catalyst/benchmarks/VarkaEmitterParityBenchmark-jdk25-results.txt # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/varka/VarkaChronoSuite.scala
Merging master brought in the perfect-hash leap flag, which changes add_months's emitted bytes, so the file was regenerated once on the merged code rather than carrying a measurement half from each side. Everything reproduces. add_months reads 634.3/632.4 M rows/s against the 642.4/ 629.4 the leap-flag PR committed - unmoved, as it should be, since this task does not touch that node. The in-run control, per-row LocalDate year, reads 481.3, the same band it has held all session. The year A/B is 2172.3 against 2164.8 null-free, and 2131.3 against 2167.7 mixed - which is the *kept* side ahead by 1.7%. PLAN_TASK_48.md 9.1 already records that the sign is not stable, two of five runs having flipped on that pair, so this is the section's own claim showing up in the artifact rather than a contradiction of it. A sentence there now says so, because a reader comparing the committed file against the table deserves to be told which one is the measurement and which is a single sample. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
`VarkaVectorApiProbeBenchmark` has no blank line between its java and scala import groups, which scalastyle rejects: "There should at least one a single empty line separating groups java and scala." The file came from the gather-probe PR and merged with the error in it, because the fork's Actions runs were being cancelled wholesale at the time and no linter check ever reported. It surfaced on the first branch whose CI actually ran after merging master. Worth noting for its own sake: for a stretch this session, a green-looking PR meant "nothing reported", not "nothing failed" - `dev/scalastyle` locally is the check that would have caught this without CI. Fixed on this branch rather than only where it was noticed, since every open branch carries the same merged file and would otherwise fail the same way. `dev/scalastyle` is clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3VxmqDKhWHkvQ4Jrp6coG
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.
What changes were proposed in this pull request?
In the PR, I propose a
yearthat does not compute the month, in four commits (plan, identity, elision, measurement), plus the plan for a follow-on task the work turned up.The year tail needs one bit out of the March-based month
mp: whether the March year has turned January, which ismp >= 10. Sincemp = (5 * doy + 2) / 153is exact over the whole domain the prefix leaves, that bit isdoy >= 306— an integer identity, proved in three lines and then asserted over all 366 values of the domain through the shipped constants. SoemitChronoYearreads the day of year, and the prefix's month step becomes dead work in any body whose calendar tails are all years.PLAN_MILESTONE_4.md2.18 predicted this task and task 32 step B were in tension: once the prefix is shared it computesmpfor the month, day-of-month and quarter tails regardless, so "whichever of the two lands second inherits the smaller half of the win" — one op instead of five. Step B1 landed first. This does not take the smaller half. The step is dead work exactly when no consumer readsmarchMonth, so it becomes conditional on a plan-time consumer set: a year-only loop method takes the full win (the TPC-H q7/q8/q9 shape),year(d), month(d)in one method correctly keeps it, anddayofyear(#64) andtrunc(d, 'YEAR')(task 35) inherit the elision for free since both testdoy >= 306themselves.The scope of that consumer set is the one place the plan was wrong, and it is worth reading the code for. The plan said "per body", on the belief that
Slotsis planned per body method. It is not —planSlotsreceives the kernel's whole output list, and it is the group threaded intoemitBody/emitLaneGroupthat narrows a loop method to its own outputs. A body-scoped set therefore kept the month step in ayear(d)loop method merely becausemonth(d)was another output of the same kernel, losing the elision entirely."sharing the prefix leaves every loop method byte for byte as it was"caught it on the first run. The set is now computed per lane group, beside theemittedFragments.clear()that has exactly that scope — which is also what makes it sound, since a fragment is re-earned in each lane group and what must hold is that every reader oft[5]in a group is preceded by a write of it in that group. Reading the set rather than the node being emitted is what keeps the decision order-independent under sharing.Measured, by minimums over five regenerations at each width:
year, null-freeyear, mixed nullsyear, null-freeyear, mixed nullsInside noise, and the sign is not stable — in two of five runs at each width the kept side won the mixed pair. That is the outcome the plan registered before measuring: four ops off a 43-op body at a 9 ms best time is under half a millisecond, below what the file resolves. The default ships on the step being provably dead work where it is elided and on the exhaustive sweep; the number is recorded because the repo's rule is that a performance claim traces to a committed file. The deterministic deliverable is the op count, asserted off the class file: a year-only loop body goes from 43 to 39
IntVectorinvocations.The regeneration also found something that is not this task.
year, null-freemoves 1823.4 → 2166.5 M rows/s in the committed file, a fifth, andyear+month, separateandyear(d1), year(d2)move with it. Three things separate that from the 1.01x above: the in-run controlper-row LocalDate year, which no Varka change touches, reads 481.1–481.6 against a committed 479.4, so the machine had not drifted; the in-run A/B puts this task's share at 1.01x; and the results file was last regenerated at06d96642707(task 32 step B1), after which exactly two commits touched the emitter —71ebc645605, task 51's removal of the per-extraction range guard, andcb176a077eb, task 38's column offsets, which no calendar kernel runs. By elimination it is task 51's, and the shape fits: the guard was paid once per calendar node, so a one-field kernel gains a fifth while the four-field shared kernel, paying it once for four tails, does not move at all (799.8 → 797.7). So task 51 shipped a ~19% win to every single-field calendar kernel and never regenerated the parity file — recorded inPLAN_TASK_48.md9.2, generalised inSKILLS.md, and flagged inPLAN_TASK_45.md, whose baseline column was stale in every calendar row.The epilogue's unshared
HugeMethodLimitcrossing moves from 19 outputs to 20; shared stays at 44. That is the third move of that number for the third unrelated reason, so the full ladder is re-measured and appended toPLAN_TASK_32.md7.1 beside the other two rather than overwriting them.Also included:
PLAN_TASK_53.md, a plan only, no code. Velox was read alongside this lowering while the work was in progress, and its scalar decomposition turns out not to follow Hinnant'scivil_from_days— which task 26 lowered — but Neri-Schneider 2022. One half of that paper reaches anIntVectorlane and the other does not, and the plan separates them with an admission check of the kind task 26 established. Takeable: the month index and day of month from a single affine numerator, verified over the whole domain against the forms Varka ships (zero mismatches at all 366 values), with the one division it needs having an exact in-lane magic checked at all 65536 values — worth −2 ops onmonth, −4 ondayofmonth, −3 ontrunc(d, 'MONTH'), andyearunchanged. Not takeable: the paper's correction-free century (a 2^26 dividend against a multiplier no larger than 32) and its year step (the high half of a 64-bit product, andVectorOperatorshas no multiply-high on any lane type). Those wait for task 49's int64 lanes. Also noted there, before it is discovered: task 53 shrinks task 48's elision from four ops to two.Why are the changes needed?
year(date)is the one calendar extraction the headline corpus asks for — TPC-H q7, q8 and q9 use it and nothing else — and it was computing a month no tail read. The milestone predicted the win would mostly evaporate once the prefix was shared; making the step conditional on its actual consumers recovers it, and generalises to every other tail that does not read the month.Does this PR introduce any user-facing change?
No. Emitted-code and plan-document changes only; the answers are unchanged, which the exhaustive sweep is what establishes.
How was this patch tested?
Every Varka suite in catalyst and sql, at both vector widths (
-XX:MaxVectorSize=16through the forked test JVM'sTest / javaOptions), sweeps included.The exhaustive sweep was extended rather than merely re-run: it now covers a year-only kernel under both sharing modes and both switch positions, over all 16,777,216 covered days, because the four-field shape keeps the month step under sharing and so never sweeps an elided prefix.
VarkaChronoSuite's own sweep over the same range is unchanged and green, which is the milestone's first validation criterion for this task.New bounded tests: the 366-case identity; the op count off the class file, through a new
VarkaEmitterTestSupport.invocationCount; order independence, whereSeq(Year, Month)andSeq(Month, Year)under sharing both keep the step in the epilogue and both elide in exactly one loop method; and the unshared per-node case, where the year's own prefix elides and the month's does not.Predictions scored in
PLAN_TASK_48.md9.3: 2, 3, 4 and 5 held; 1 missed in the absolute only (43 → 39 ops, not 45 → 41 — the delta was exact, the prediction counted twoVectorMaskops the counter does not); 6 carries forward to whichever of this and #64 merges second.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)