Skip to content

Commit ca2d9e0

Browse files
rubysclaude
andcommitted
fix(spinel): column_value hands back native types — and unpin pluck's RBS
The FFI Db shim returned every column as text, a placeholder its own comment flagged as "until per-column sqlite3_column_type dispatch is wired". Both gem-backed shims (db_cruby, db_jruby) already answered the native-type contract, and sqlite_adapter's select_rows comment already claimed all of them did. Wire the dispatch so the claim is true. Two changes, because the first alone did nothing: 1. Db.column_value dispatches on sqlite3_column_type (storage class), returning Integer/Float/String/nil. Storage class, NOT decltype: the declared type diverges from the gem shims exactly when the two disagree, and it cannot detect NULL at all — which is the whole reason the nullable reads need a per-row probe. Same price column_int_opt already pays. 2. Relation's RBS pinned pluck to Array[String] and pick to String?, which FORCED the coercion straight back — measured, still String after change 1. Both are untyped now, which is what the catalog (src/catalog/mod.rs, ArrayOfUntyped) already said; the sidecar was the outlier. Measured on the lobsters AOT lane: recent_threads' pluck(:thread_id) went from "19948" to 19948, so the thread_ids it returns once again match the Integer keys model attributes carry. Also closes a second silent one on a different path — tag.rb's all_with_filtered_counts_for looks up counts[t.id] against group(:tag_id).group_count, whose keys were Strings, so filtered_count read 0 for every tag. /threads itself stays empty: it is blocked behind matz/spinel#3505, a nil returned through a block typed Integer? that is a different Hash key than a literal nil. This clears the layer behind that one. All 26 lobsters routes byte-identical before/after; cargo test green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6911b5a commit ca2d9e0

4 files changed

Lines changed: 77 additions & 20 deletions

File tree

runtime/ruby/active_record/relation.rbs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,24 @@ module ActiveRecord
6262
def size: () -> Integer
6363
def delete_all: () -> Integer
6464
def update_all: (untyped updates) -> Integer
65-
def pluck: (untyped col) -> Array[String]
66-
def pick: (untyped col) -> String?
65+
# `pluck`/`pick` hand back whatever the COLUMN holds — Integer for an
66+
# INTEGER column, Float for REAL, String for TEXT, nil for NULL —
67+
# because they read straight through `select_rows` without a cast.
68+
# Pinning these to String used to be harmless: the spinel FFI shim
69+
# returned everything as text anyway, so the seed and the runtime
70+
# agreed. Once `Db.column_value` began answering the native-type
71+
# contract the other shims already met, the String pin became the
72+
# thing FORCING the coercion back — `pluck(:thread_id)` handed back
73+
# `"19948"` where the model attribute held `19948`, and every Hash
74+
# lookup between them missed (lobsters /threads).
75+
#
76+
# `untyped` is the honest type at this level: the column's type is
77+
# not knowable from `Relation`. Typing it precisely means the
78+
# lowerer monomorphizing per call site from the schema, the way
79+
# `ids` below can be Array[Integer] because it only ever reads a
80+
# primary key.
81+
def pluck: (untyped col) -> Array[untyped]
82+
def pick: (untyped col) -> untyped
6783
def ids: () -> Array[Integer]
6884
def find: (Integer id) -> untyped
6985
def find_by: (untyped conditions) -> untyped

runtime/spinel/db.rb

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ module SQL
5555
ffi_const :OK, 0
5656
ffi_const :ROW, 100
5757
ffi_const :DONE, 101
58+
# sqlite3_column_type storage classes. NULL is the one the nullable
59+
# reads have always needed; the rest let `column_value` hand back the
60+
# driver's native type instead of everything-as-text.
61+
ffi_const :INTEGER_TYPE, 1
62+
ffi_const :FLOAT_TYPE, 2
63+
ffi_const :TEXT_TYPE, 3
64+
ffi_const :BLOB_TYPE, 4
5865
ffi_const :NULL_TYPE, 5
5966
# open_v2 flags: READWRITE | CREATE | URI. The first two are what plain
6067
# `sqlite3_open` uses, so a non-URI path opens identically either way.
@@ -569,18 +576,48 @@ def self.column_text(stmt, i)
569576
end
570577
end
571578

572-
# Raw typed column read — the gem-backed shims (db_cruby/db_jruby)
573-
# return the driver's native value (Integer/Float/String/nil). The
574-
# FFI shim keeps the text representation until per-column
575-
# `sqlite3_column_type` dispatch is wired; NULL at least maps to nil
576-
# (not "") so nil-keyed grouping and truthiness behave. Same
577-
# copy-out as column_text (the buffer dies at the next step).
579+
# Raw typed column read — the driver's native value, the same contract
580+
# the gem-backed shims (db_cruby/db_jruby) answer: Integer for an
581+
# INTEGER storage class, Float for REAL, String for TEXT, nil for NULL.
582+
#
583+
# Dispatches on the STORAGE CLASS of the value in THIS row, not on the
584+
# column's declared type. SQLite is dynamically typed and both gem
585+
# shims report what is actually stored, so storage class is what keeps
586+
# the AOT tree's hydration in step with the CRuby lane. Caching
587+
# `sqlite3_column_decltype` once per statement would save the probe
588+
# below, but it would diverge from the shims exactly when storage class
589+
# and declaration disagree — and it cannot detect NULL at all, which is
590+
# the whole reason the nullable reads need a per-row probe.
591+
#
592+
# Costs one `sqlite3_column_type` call per column per row on top of the
593+
# value read. That is the same price `column_int_opt` and friends
594+
# already pay, and it is unavoidable: `sqlite3_column_int` cannot tell
595+
# a stored 0 from NULL. Before this, every value arrived as text, so
596+
# `pluck(:thread_id)` handed back `"19948"` where the model attribute
597+
# held `19948` and every Hash lookup between them missed.
598+
#
599+
# Integers read through `sqlite3_column_int` (32-bit), matching
600+
# `column_int` and the `from_stmt` hydration path; a corpus with keys
601+
# past 2^31 would need `sqlite3_column_int64` wired for all of them
602+
# together, not just here. BLOB falls to the text read — no corpus
603+
# caller stores one, and text is a more useful stand-in than raising.
578604
def self.column_value(stmt, i)
579-
s = SQL.sqlite3_column_text(stmt, i)
580-
if s.nil?
605+
t = SQL.sqlite3_column_type(stmt, i)
606+
if t == SQL::NULL_TYPE
581607
nil
608+
elsif t == SQL::INTEGER_TYPE
609+
SQL.sqlite3_column_int(stmt, i)
610+
elsif t == SQL::FLOAT_TYPE
611+
SQL.sqlite3_column_double(stmt, i)
582612
else
583-
s + ""
613+
# Same copy-out as column_text — the libsqlite3 buffer is
614+
# invalidated by the next step or finalize on this stmt.
615+
s = SQL.sqlite3_column_text(stmt, i)
616+
if s.nil?
617+
nil
618+
else
619+
s + ""
620+
end
584621
end
585622
end
586623

runtime/spinel/sqlite_adapter.rb

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,9 @@ def self.truncate(table)
9292
# keyed by column name. Values read through `Db.column_value` — the
9393
# driver's NATIVE types (Integer/Float/String, nil for NULL), which
9494
# is what ActiveRecord hands the app: `group_by(&:fk)[nil]` finds
95-
# root rows, integer columns compare as integers. (The FFI Db shim
96-
# still returns text values, nil-for-NULL aside — see db.rb.) Each
95+
# root rows, integer columns compare as integers, and `pluck(:fk)`
96+
# returns keys that match the ones model attributes carry. All three
97+
# shims answer this now, the FFI one by storage-class dispatch. Each
9798
# row is its own Hash so the caller gets a stable copy after
9899
# `finalize`.
99100
def self.select_rows(sql)

src/emit/ruby/expr.rs

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -389,13 +389,16 @@ fn emit_cast(value: &Expr, target_ty: &crate::ty::Ty) -> String {
389389
// than a missed narrowing.
390390
//
391391
// Adapters disagree about what a boolean column reads as: the
392-
// gem-backed shims give `true`/`false` or `1`/`0`, while
393-
// spinel's `Db.column_value` is `sqlite3_column_text`, so every
394-
// value arrives as a STRING — `"0"` for false. Assigning that
395-
// raw string into a slot the RBS pins `bool` makes it `true`,
396-
// and lobsters' `Rack::MiniProfiler.authorize_request if @user
397-
// && @user.is_admin?` then fired for a user whose `is_admin` is
398-
// 0, taking the whole request down under spinel AOT.
392+
// gem-backed shims give `true`/`false` or `1`/`0`, and spinel's
393+
// `Db.column_value` gives the `0`/`1` of the INTEGER storage
394+
// class (it used to give the STRING `"0"`, before storage-class
395+
// dispatch landed in runtime/spinel/db.rb — the string spelling
396+
// stays covered below because nothing guarantees a future
397+
// adapter won't reintroduce it). Assigning a raw `"0"` into a
398+
// slot the RBS pins `bool` makes it `true`, and lobsters'
399+
// `Rack::MiniProfiler.authorize_request if @user &&
400+
// @user.is_admin?` then fired for a user whose `is_admin` is 0,
401+
// taking the whole request down under spinel AOT.
399402
//
400403
// Compare the string form so one expression covers every
401404
// adapter: `false`/`0`/`"0"` are the false spellings, plus `""`

0 commit comments

Comments
 (0)