Skip to content

Commit d7da28a

Browse files
mstrathmanclaude
andcommitted
fix: close the fail-loud gaps the docs audit found
Three defects surfaced by executing every documented example: distill_forecast silently ignored unknown option keys (it reads values via json_extract with no closed key list), so a misspelled option trained a student with defaults instead of erroring. It now validates against a closed key list through the shared options parser before extraction, matching every other operation's contract. predict() with a distilled student silently ignored a non-NULL train_query: the caller thinks their conditioning rows matter and they do not. A student named alongside a train_query is now rejected (PREDICT_ERR_OPTIONS) with the fix in the message; NULL serves as before. backtest's model error message omitted tsb from the accepted list even though tsb is accepted. Regression tests for the first two; the interim doc qualifications ("distill_forecast is the one exception", "the query is not read") are reverted to the strict contract they now describe. 185/220 tests pass on plain/onnx builds, ASan clean, amalgamation compiles, site builds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 83c5494 commit d7da28a

7 files changed

Lines changed: 66 additions & 6 deletions

File tree

‎predict-distill.c‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1986,6 +1986,23 @@ static int fdistill_train_teacher(sqlite3 *db, const char *tq,
19861986
}
19871987
#endif /* SQLITE_PREDICT_ONNX */
19881988

1989+
/* distill_forecast option keys. Values are extracted with json_extract
1990+
* below; this closed list exists so an unknown or misspelled key fails
1991+
* loudly (the same contract as every other operation) instead of being
1992+
* silently ignored. */
1993+
static const char *const FCST_DIST_OPTION_KEYS[] = {
1994+
"context", "horizon", "student_id", "hidden",
1995+
"epochs", "lr", "teacher", "quantiles", NULL};
1996+
1997+
static int fdist_opt_validate_cb(void *ctx, const char *key,
1998+
sqlite3_value *value, char **errmsg) {
1999+
UNUSED_PARAMETER(ctx);
2000+
UNUSED_PARAMETER(key);
2001+
UNUSED_PARAMETER(value);
2002+
UNUSED_PARAMETER(errmsg);
2003+
return 0; /* keys validated by the parser; values read via json_extract */
2004+
}
2005+
19892006
/* ---- distill_forecast vtab ---- */
19902007

19912008
#define FD_MODEL 0
@@ -2110,6 +2127,17 @@ static int fd_filter(sqlite3_vtab_cursor *pCur, int idxNum, const char *idxStr,
21102127
if (!tq)
21112128
FD_FAIL("%s: train_query is required", PREDICT_ERR_SCHEMA);
21122129
{
2130+
/* reject unknown option keys before extracting the known ones */
2131+
char *operr = NULL;
2132+
if (predict0_options_parse(db, options, FCST_DIST_OPTION_KEYS,
2133+
fdist_opt_validate_cb, NULL, &operr)) {
2134+
sqlite3_free(vtab->base.zErrMsg);
2135+
vtab->base.zErrMsg = operr;
2136+
sqlite3_free(student_id);
2137+
sqlite3_free(levels);
2138+
sqlite3_free(teacher);
2139+
return SQLITE_ERROR;
2140+
}
21132141
sqlite3_stmt *op = NULL;
21142142
if (sqlite3_prepare_v2(
21152143
db,

‎predict-forecast.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2532,7 +2532,7 @@ static int bt_filter(sqlite3_vtab_cursor *pCur, int idxNum, const char *idxStr,
25322532
if (!is_auto && !fixed_model) {
25332533
int rc = bt_error(cur, PREDICT_ERR_MODEL_NOT_FOUND,
25342534
"backtest() supports the statistical models "
2535-
"(theta-classic, stub-seasonal-naive) or auto: ",
2535+
"(theta-classic, stub-seasonal-naive, tsb) or auto: ",
25362536
opts.model);
25372537
forecast_opts_free(&opts);
25382538
return rc;

‎predict-tabular.c‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,16 @@ static int pr_filter(sqlite3_vtab_cursor *pCur, int idxNum,
395395
return rc;
396396
}
397397
if (m.runtime && strcmp(m.runtime, "tree") == 0) {
398+
if (train_sql) {
399+
/* a student carries its training in its blob; a train_query
400+
* here would be silently ignored, so reject it loudly */
401+
predict0_model_row_free(&m);
402+
rc = pr_error(cur, PREDICT_ERR_OPTIONS,
403+
"a distilled student takes no train_query; pass NULL: ",
404+
model_id);
405+
pred_opts_free(&opts);
406+
return rc;
407+
}
398408
rc = run_student(cur, db, model_id, apply_sql, &m, &opts);
399409
} else if (m.runtime && strcmp(m.runtime, "onnx") == 0) {
400410
#ifdef SQLITE_PREDICT_ONNX

‎tests/test_distill.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,3 +436,17 @@ def test_corrupt_student_blob_errors_not_crashes(db):
436436
db.execute("SELECT * FROM predict(NULL,'SELECT id, f1, f2 FROM a',"
437437
" json_object('model','bad'))").fetchall()
438438
assert "PREDICT_ERR" in str(e.value)
439+
440+
441+
def test_student_rejects_train_query(db):
442+
# a student carries its training in its blob: a train_query alongside
443+
# it would be dead weight, so it is rejected, never silently ignored
444+
db.execute("CREATE TABLE tt(f1 REAL, f2 REAL, label TEXT)")
445+
db.executemany("INSERT INTO tt VALUES (?,?,?)",
446+
[(i * 0.3, (i * 7) % 5, "c%d" % (i % 2)) for i in range(60)])
447+
db.execute("SELECT * FROM distill_predict('SELECT f1, f2, label FROM tt',"
448+
" '{\"target\":\"label\",\"student_id\":\"guard-s\"}')").fetchone()
449+
with pytest.raises(sqlite3.OperationalError, match="no train_query"):
450+
db.execute("SELECT * FROM predict('SELECT f1, f2, label FROM tt',"
451+
" 'SELECT rowid, f1, f2 FROM tt',"
452+
" '{\"model\":\"guard-s\"}')").fetchall()

‎tests/test_forecast_student.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,3 +328,13 @@ def test_auto_stays_pure_without_a_registry(db):
328328
assert doc["model"] in ("theta-classic", "stub-seasonal-naive", "tsb")
329329
assert db.execute("SELECT count(*) FROM sqlite_master WHERE name LIKE"
330330
" '_predict%'").fetchone()[0] == 0
331+
332+
333+
def test_distill_forecast_rejects_unknown_option_key(db):
334+
# the fail-loud options contract applies to distill_forecast too
335+
ncol = _load_windows(db)
336+
with pytest.raises(sqlite3.OperationalError, match="OPTIONS"):
337+
db.execute(
338+
f"SELECT * FROM distill_forecast('SELECT {_cols(ncol)} FROM w',"
339+
f" json_object('context',{L},'horizon',{H},"
340+
f"'student_id','x','bogus',1))").fetchall()

‎website/src/content/docs/guides/distillation.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ SELECT row_ref, prediction, confidence FROM predict(NULL,
2727

2828
The `NULL` first argument is the signature of serving a student: `predict`
2929
normally takes a training query for in-context models, but a student carries
30-
its training inside its blob, so pass `NULL`; a query in that position is
31-
not read when a student serves the call.
30+
its training inside its blob, so passing a query alongside a student is
31+
rejected (`PREDICT_ERR_OPTIONS`) rather than silently ignored.
3232

3333
`student_kind` is `tree` (a single CART), `gbt` (a gradient-boosted forest
3434
with second-order leaves, which matches or beats tuned XGBoost on most

‎website/src/content/docs/reference/options.md‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ description: Every option each function accepts, passed as a trailing JSON objec
55

66
Options are a single JSON object in the last argument. Unknown keys and
77
wrong-typed values are rejected with `PREDICT_ERR_OPTIONS`, so a typo fails loud
8-
rather than being ignored. `distill_forecast` is the one exception today: it
9-
reads only its known keys and ignores anything else, so check its spelling
10-
yourself.
8+
rather than being ignored.
119

1210
## forecast
1311

0 commit comments

Comments
 (0)