Commit f65be0e
authored
feat(appkit): infer numeric SQL type for sql.number(), add typed variants (#323)
* feat(appkit): infer numeric SQL type for sql.number(), add typed variants
Currently sql.number() unconditionally produces __sql_type: "NUMERIC",
which Databricks SQL binds as DECIMAL(10,0). That breaks LIMIT and
OFFSET — Spark requires an integer-typed expression and rejects the
parameter with INVALID_LIMIT_LIKE_EXPRESSION.DATA_TYPE — and silently
truncates non-integer JS numbers (sql.number(3.14) sent value="3.14"
into a DECIMAL(10,0) slot).
Two changes:
1. sql.number() now infers the wire type from the value:
- integer JS number -> BIGINT
- non-integer JS number -> DOUBLE
- numeric string -> NUMERIC (preserves caller's precision intent;
a string is an explicit choice to skip JS-number coercion)
Picks the most-precise type the value can losslessly represent.
Spark coerces numeric types implicitly so existing queries against
BIGINT/DECIMAL/DOUBLE columns keep working, plus LIMIT now accepts
sql.number(10) directly.
2. Typed variants for callers who need to override the inference:
- sql.int(value) -> INT
- sql.bigint(value) -> BIGINT (also accepts JS bigint for values
beyond Number.MAX_SAFE_INTEGER)
- sql.float(value) -> FLOAT
- sql.double(value) -> DOUBLE
- sql.decimal(value) -> NUMERIC (decimal precision preserved)
sql.int() and sql.bigint() reject non-integer inputs at the
helper boundary so the failure surfaces early, before the wire.
SQLNumberMarker.__sql_type widens from "NUMERIC" to a union of the
numeric SQL types. Non-breaking for callers: any code that previously
held an SQLNumberMarker still type-checks (the union is wider in
return position). The two unit tests that pinned `type: "NUMERIC"`
for sql.number(integer) are updated to expect BIGINT.
Discovered while building a parameterized analytics query against
LIMIT — the bare `LIMIT :limit` case is the most visible failure but
the underlying issue affects any query where the column type matters.
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
* docs: regenerate sql.* API reference for typed numeric variants
Co-authored-by: Isaac
* fix(shared): address review feedback on typed numeric SQL variants
P1 fixes
- Reject JS integers outside Number.MAX_SAFE_INTEGER in sql.number(),
sql.int(), and sql.bigint(number). The marker would otherwise advertise
BIGINT for a value already lost to JS-double precision.
- Widen sql.number("10") to BIGINT for integer-shaped strings so handler
code that passes req.query strings works with LIMIT/OFFSET. Decimal-
shaped strings still emit NUMERIC.
- Type-generator: extract INT/BIGINT/TINYINT/SMALLINT/FLOAT/DOUBLE/DECIMAL
-- @param annotations, give them sensible defaults, and route each SQL
type to its closest typed helper in sqlTypeToHelper.
P2 fixes
- Reject Infinity / -Infinity / NaN, hex / scientific-only / whitespace
strings via a strict NUMERIC_LITERAL_RE.
- Emit BIGINT wire values via BigInt(value).toString() so 1e15 -> "1000000000000000"
rather than exponent text.
- Bound-check sql.int (32-bit signed) and sql.bigint (64-bit signed)
inputs; surface a descriptive error pointing to the wider helper.
- Narrow each typed helper's return type to the exact __sql_type literal.
- Add bound, boundary, error, and precision-loss tests for the typed
helpers, plus an integration test for LIMIT :n OFFSET :m bindings.
- Add @returns and @example for every new helper; regenerate
docs/docs/api/appkit/Variable.sql.md.
- Rename sql.decimal -> sql.numeric so name matches the wire literal
(existing typed helpers all match name -> wire). Update analytics.md
to mention the new helpers and refresh the LIMIT example.
Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
* fix(shared): address ACE multi-model review findings
Two confirmed issues from a follow-up multi-model review of the
prior commit's typed numeric variants:
- sql.number(integer-shaped string) widened to BIGINT without
bounds-checking. "9223372036854775808" overflowed the 64-bit
wire type silently. Now runs the same BIGINT range check as
sql.bigint() and throws with a hint to sql.numeric() for
arbitrary-precision integers.
- The @param extraction regex matched TIMESTAMP_NTZ as TIMESTAMP,
losing the NTZ specificity. Reordered the alternation so
TIMESTAMP_NTZ wins, added a \b boundary, and gave it a default
literal in defaultForType.
Plus drive-bys:
- Remove stale sql.interval() reference in SQLTypeMarker JSDoc.
- Pin behaviour with boundary tests at +/- 2^63 and a regression
test that TIMESTAMP_NTZ is not partially matched.
Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
* fix(shared): sql.number infers INT (not BIGINT) for LIMIT/OFFSET compatibility
Empirical testing against e2-dogfood.staging (from @calvarjorge) showed
that Spark's LIMIT/OFFSET operators require IntegerType specifically —
LongType/BIGINT is rejected with INVALID_LIMIT_LIKE_EXPRESSION.DATA_TYPE:
The offset expression must be integer type, but got "BIGINT".
So the original PR's "LIMIT now Just Works" claim was false: BIGINT
fails for the exact motivating case. Catalyst auto-widens INT to
BIGINT/DECIMAL/DOUBLE for wider columns, so defaulting to INT is
strictly better than defaulting to BIGINT.
Change sql.number inference:
- JS integer in [-2^31, 2^31 - 1] → INT (was BIGINT)
- JS integer outside INT but within MAX_SAFE_INTEGER → BIGINT
- integer-shaped string in INT range → INT (was BIGINT)
- integer-shaped string outside INT, within BIGINT → BIGINT
- everything else unchanged (DOUBLE for non-integer, NUMERIC for
decimal strings, throw for out-of-BIGINT and non-numeric)
Update analytics.md to recommend `-- @param limit INT` and explain
the Spark IntegerType requirement. Update unit + integration tests
to pin INT bindings for in-range values, with explicit boundary
coverage at +/- 2^31. Regenerate the API reference page.
Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
* docs(shared): document how to re-validate LIMIT/OFFSET mocked tests against prod
The LIMIT/OFFSET integration tests assert wire-type strings produced
by sql.number — they don't round-trip against a real warehouse. The
original PR claimed BIGINT worked for LIMIT and tests passed, but
production Spark rejected the binding.
Add a comment in the test block with a copy-pasteable
/api/2.0/sql/statements payload, the expected success and failure
states, and the exact error string. If Spark's LIMIT type contract
ever changes, this comment is the smoke test to catch it before the
mocked assertions silently drift away from production behaviour.
Verified empirically against two independent SQL Warehouses on
e2-dogfood.staging (including dd43ee29fedd958d, the warehouse the
original PR cited as evidence): INT succeeds with 2 rows, BIGINT
fails with [INVALID_LIMIT_LIKE_EXPRESSION.DATA_TYPE] ... SQLSTATE: 42K0E.
Co-authored-by: Isaac
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>
---------
Signed-off-by: James Broadhead <jamesbroadhead@gmail.com>1 parent 7a679d6 commit f65be0e
10 files changed
Lines changed: 927 additions & 63 deletions
File tree
- docs/docs
- api/appkit
- plugins
- packages
- appkit/src
- plugins/analytics/tests
- type-generator
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
270 | 270 | | |
271 | 271 | | |
272 | 272 | | |
273 | | - | |
| 273 | + | |
274 | 274 | | |
275 | 275 | | |
276 | 276 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
5 | 8 | | |
6 | 9 | | |
7 | 10 | | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
8 | 20 | | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
9 | 24 | | |
10 | 25 | | |
11 | 26 | | |
| |||
15 | 30 | | |
16 | 31 | | |
17 | 32 | | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
18 | 70 | | |
19 | 71 | | |
20 | 72 | | |
| |||
134 | 186 | | |
135 | 187 | | |
136 | 188 | | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
137 | 289 | | |
138 | 290 | | |
139 | 291 | | |
140 | 292 | | |
141 | 293 | | |
142 | 294 | | |
143 | | - | |
144 | | - | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
145 | 316 | | |
146 | 317 | | |
147 | 318 | | |
| |||
153 | 324 | | |
154 | 325 | | |
155 | 326 | | |
156 | | - | |
| 327 | + | |
157 | 328 | | |
158 | | - | |
| 329 | + | |
159 | 330 | | |
160 | 331 | | |
161 | | - | |
162 | | - | |
| 332 | + | |
| 333 | + | |
| 334 | + | |
| 335 | + | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
163 | 345 | | |
164 | 346 | | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
165 | 370 | | |
166 | | - | |
167 | | - | |
| 371 | + | |
168 | 372 | | |
169 | 373 | | |
170 | 374 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
46 | | - | |
| 46 | + | |
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
52 | 58 | | |
53 | | - | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
54 | 63 | | |
55 | 64 | | |
56 | 65 | | |
| |||
0 commit comments