From ff72f4bcbabf472304d8ada8b72927f55a84444a Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Sat, 11 Jul 2026 10:32:40 +0200 Subject: [PATCH] fix(cli): friendlier ingest/delete preflight messages + validated --schema types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small ingest/preflight polish tickets, one coherent change. The data INGESTOR owns validation; the CLI only PREVIEWS it (Principle 6), so every new rule here mirrors an authoritative ingestor grammar rather than inventing a CLI-only one. - #215: derive the "Supports N tasks" count from the task registry (push.SupportedCategoryIDs) instead of a stale hardcoded "9", and fix the text example to show sequences/ for masked_language_modeling (its primary_subdir per layout.v1.json), both read from the vendored contract so they can't drift again. - #214: a tabular task with no --label-column now gets the friendly flag-naming message (listing the CSV's columns) via a targeted pre-check, instead of the opaque label-oneOf schema dump. Only the missing case is intercepted; other schema errors still surface. - #213: validate each --schema TYPE token locally against the ingestor's REAL accepted set (mirrors database.py::_get_sqlalchemy_type, di#349) so a bogus type (e.g. age:BANANA) is caught before the upload — on both the --schema flag path (ParseSchema) and `data validate`. Also fixes ParseSchema's comma-split so DECIMAL(p,s)/NUMERIC(p,s) — types the ingestor accepts — parse as one entry instead of being torn apart. - #76: (a) `data delete ""` gives a delete-appropriate positional-arg message, not the ingest-path "set --name"; (b) --number-of-keypoints distinguishes unset ("requires") from an explicit non-positive value ("must be a positive integer (got N)") via cmd.Flags().Changed; (c) suppress the redundant parent-level "label: got object, want string" oneOf type-noise when a specific label.policy error is present. Tests added/updated for each fix across internal/cli, internal/push, and internal/schema. go build/vet/test/gofmt all clean; coverage floors hold (internal/cli 74.4%). Closes #213 #214 #215 #76 Co-Authored-By: Claude Opus 4.8 --- internal/cli/data.go | 80 ++++++++++++++++++++-- internal/cli/data_delete.go | 9 +++ internal/cli/data_test.go | 104 ++++++++++++++++++++++++++++ internal/cli/ingest.go | 48 ++++++++++++- internal/cli/ingest_test.go | 60 ++++++++++++++++ internal/push/tabular.go | 113 +++++++++++++++++++++++++++++-- internal/push/tabular_test.go | 75 ++++++++++++++++++++ internal/schema/validate.go | 47 ++++++++++++- internal/schema/validate_test.go | 98 +++++++++++++++++++++++++++ 9 files changed, 619 insertions(+), 15 deletions(-) diff --git a/internal/cli/data.go b/internal/cli/data.go index f857abad..b9a93c35 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -9,6 +9,7 @@ import ( "k8s.io/client-go/kubernetes" "os" "path/filepath" + "sort" "strings" "github.com/spf13/cobra" @@ -143,12 +144,17 @@ func newDataIngestCmd() *cobra.Command { Use: "ingest ", Aliases: []string{"push"}, Short: "Ingest a local dataset into your workspace", - Long: `Ingests a local dataset into your workspace's storage, + // The task COUNT and the text-family subdir names are derived from the + // registry / vendored layout contract (push.SupportedCategoryIDs + + // push.TextSidecarDir) rather than hardcoded, so the help can't drift + // from what the CLI actually supports (cli#215): the count used to read + // a stale "9", and the text example showed texts/ for every text task + // even though masked_language_modeling stages into sequences/. + Long: fmt.Sprintf(`Ingests a local dataset into your workspace's storage, submits the ingestion run, and follows it to completion (streaming progress + the final summary). Your data never leaves your own -infrastructure. Supports 9 tasks (image classification, -object/keypoint detection, text classification, masked language -modeling, and the tabular / time-series family); pick one with --task. +infrastructure. Supports %[1]d tasks across the image, text, and +tabular / time-series families; pick one with --task. is the data itself. What it looks like depends on the task: @@ -170,11 +176,11 @@ modeling, and the tabular / time-series family); pick one with --task. ... text (classification, masked language modeling) — a folder with - labels.csv + a texts/ subfolder: + labels.csv + a %[2]s/ subfolder (masked language modeling uses %[3]s/): reviews/ labels.csv (required) - texts/ (required) + %[2]s/ (required — %[3]s/ for masked language modeling) 001.txt ... @@ -205,6 +211,9 @@ Exit codes: 8 jobs-manager rejected the submit (4xx/5xx other than auth) 9 ingestion Job exited non-zero, or completed with row-level failures the summary panel reports`, + len(push.SupportedCategoryIDs()), + push.TextSidecarDir("text_classification"), + push.TextSidecarDir("masked_language_modeling")), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { var localPath string @@ -229,6 +238,14 @@ Exit codes: // unset task now drives the picker (TTY) or a clear error // (non-interactive), never a silent image assumption. taskSet := cmd.Flags().Changed("task") || cmd.Flags().Changed("category") + // Record whether --number-of-keypoints was explicitly passed, so + // the keypoint set-vs-unset message (#76b) can distinguish an + // explicit zero value from an unset flag (both look like the Go + // zero value in the spec). + changedFlags := map[string]bool{} + if cmd.Flags().Changed("number-of-keypoints") { + changedFlags["number-of-keypoints"] = true + } // Guided mode: on a terminal (and unless --no-input), prompt // for whatever's still missing. Off a TTY / with --no-input, // prompter stays nil and runDataIngest keeps flag-only @@ -272,6 +289,7 @@ Exit codes: Interactive: interactive, Prompter: pr, TaskSet: taskSet, + ChangedFlags: changedFlags, OutputJSON: outputJSON, JSONOut: jsonOut, }) @@ -381,6 +399,15 @@ type runDataIngestArgs struct { Prompter prompter TaskSet bool + // ChangedFlags records which CLI flags were EXPLICITLY set + // (cmd.Flags().Changed), decoupling "was it passed" from "is its value + // non-zero" — the value alone can't tell `--number-of-keypoints 0` (an + // explicit, invalid value) from an unset flag (also 0). The RunE + // populates it for --number-of-keypoints; the keypoint set-vs-unset + // diagnostic (#76b) reads it. Nil in direct-construction tests that + // don't exercise that path. + ChangedFlags map[string]bool + // OutputJSON routes human output to stderr and emits a JSON result // to JSONOut (stdout); set together by the RunE in --output-json // mode (which also forces non-interactive). @@ -394,6 +421,17 @@ type runDataIngestArgs struct { ImageDigest string } +// sortedKeys returns m's keys in sorted order — used to list a CSV's inferred +// columns in the friendly missing-label message (#214) deterministically. +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // expandHome expands a leading ~ (current user or ~user) to a home // directory, leaving every other path untouched. It's the CLI-local // name for the shared pathutil.ExpandHome; cluster.expandPath resolves @@ -713,8 +751,18 @@ collaborators can train against that table without ever seeing the raw files.`)) case push.IsImage(a.Spec.Category): // keypoint_detection needs --number-of-keypoints (dataset- // specific, no default). Catch it here with an actionable - // message rather than letting the ingestor fail mid-run. + // message rather than letting the ingestor fail mid-run. Split + // UNSET from SET-BUT-INVALID (#76b): a Go 0 could mean either, so + // key on whether the flag was passed (ChangedFlags). Unset → the + // "requires" nudge; set to a non-positive value → name the bad + // value so the user sees exactly what was rejected. if a.Spec.Category == "keypoint_detection" && a.Spec.NumberOfKeypoints <= 0 { + if a.ChangedFlags["number-of-keypoints"] { + return &exitError{code: 2, err: fmt.Errorf( + "--number-of-keypoints must be a positive integer (got %d); "+ + "it's the number of keypoints per sample (e.g. 17 for COCO pose)", + a.Spec.NumberOfKeypoints)} + } return &exitError{code: 2, err: errors.New( "keypoint_detection requires --number-of-keypoints (e.g. " + "--number-of-keypoints 17); it's dataset-specific and has no default")} @@ -777,6 +825,24 @@ collaborators can train against that table without ever seeing the raw files.`)) // the registry's SelfSupervised flag (not a hardcoded id). } + // 3b. Friendly missing-label pre-check (#214). Every tabular / time-series + // task carries a label column (layout contract has_label_column=true for + // the whole family). With no --label-column the synthesized spec's + // `label` is an empty string, which trips the schema's label oneOf and + // the raw validation below dumps an opaque "got object, want string" / + // "minLength" pair. Intercept ONLY that specific missing case here — a + // label that's present-but-not-in-the-CSV still flows to + // runLocalPreflight's CheckLabelColumn, and every other schema error + // still reaches the dump — and name the flag to fix instead. + if push.IsTabular(a.Spec.Category) && a.Spec.LabelColumn == "" { + msg := "this task needs a label column, but --label-column wasn't set — " + + "pass --label-column with the name of the target column in your data CSV" + if cols := sortedKeys(a.Spec.Schema); len(cols) > 0 { + msg += " (columns: " + strings.Join(cols, ", ") + ")" + } + return &exitError{code: 2, err: errors.New(msg)} + } + // 4. Synthesize the spec from flags + validate against schema. // Catches "bad category", "missing intent" etc. BEFORE we // touch the cluster. The error formatter is the same one diff --git a/internal/cli/data_delete.go b/internal/cli/data_delete.go index 5be9f372..538def45 100644 --- a/internal/cli/data_delete.go +++ b/internal/cli/data_delete.go @@ -105,6 +105,15 @@ func runDataDelete(ctx context.Context, a runDataDeleteArgs) error { the cluster and deletes the dataset's files on the shared storage. It can't be undone — re-ingesting the data is the only way back.`) + // 0. Empty-arg guard with a DELETE-appropriate message (#76a). delete + // takes the dataset as a POSITIONAL arg, so an empty one must not fall + // through to ValidateTableName's "set --name" text (that flag belongs to + // `data ingest`, not here). ExactArgs(1) still accepts an explicit "". + if a.Table == "" { + return &exitError{code: 2, err: errors.New( + "dataset name is required — pass it as an argument: tracebloc data delete ")} + } + // 1. Validate the name before we build any PVC path from it // (push.PlanTeardown panics on an unsafe name by design). if err := push.ValidateTableName(a.Table); err != nil { diff --git a/internal/cli/data_test.go b/internal/cli/data_test.go index 1837fb59..ba82c7e1 100644 --- a/internal/cli/data_test.go +++ b/internal/cli/data_test.go @@ -14,6 +14,7 @@ import ( "github.com/tracebloc/cli/internal/cluster" "os" "path/filepath" + "strconv" "strings" "testing" ) @@ -661,3 +662,106 @@ func TestDataIngest_ScopedFlag_OnCorrectTask_NotRejected(t *testing.T) { t.Fatal("--number-of-keypoints on keypoint_detection must not be rejected as a wrong-task flag") } } + +// TestDataIngestHelp_DerivedTaskCountAndMLMSubdir pins cli#215: the task COUNT +// in the ingest help is derived from the registry (not a stale hardcoded "9"), +// and the text example reflects the layout contract — masked_language_modeling +// stages into sequences/, not texts/. +func TestDataIngestHelp_DerivedTaskCountAndMLMSubdir(t *testing.T) { + long := newDataIngestCmd().Long + n := len(push.SupportedCategoryIDs()) + want := "Supports " + strconv.Itoa(n) + " tasks" + if !strings.Contains(long, want) { + t.Errorf("help should derive the task count (%q); got:\n%s", want, long) + } + if n != 9 && strings.Contains(long, "Supports 9 tasks") { + t.Error("help still carries the stale hardcoded 'Supports 9 tasks'") + } + if !strings.Contains(long, "sequences/") { + t.Errorf("help example should show sequences/ for masked language modeling; got:\n%s", long) + } +} + +// TestDataIngest_TabularMissingLabel_Friendly pins cli#214: a tabular task with +// no --label-column gets the friendly flag-naming message (listing the CSV's +// columns), NOT the raw label-oneOf schema dump. +func TestDataIngest_TabularMissingLabel_Friendly(t *testing.T) { + root := tabularDir(t) + var buf bytes.Buffer + err := runDataIngest(context.Background(), &buf, &buf, runDataIngestArgs{ + LocalPath: root, + Spec: push.SpecArgs{Table: "t", Category: "tabular_classification", Intent: "train"}, + Printer: ui.New(&buf, ui.WithColor(false)), + }) + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("err = %v, want *exitError code 2", err) + } + msg := ee.Error() + if !strings.Contains(msg, "--label-column") { + t.Errorf("message should name --label-column; got: %q", msg) + } + for _, noise := range []string{"want string", "want object", "failed schema validation", "oneOf"} { + if strings.Contains(msg, noise) { + t.Errorf("message should be friendly, not the raw schema dump (contained %q); got: %q", noise, msg) + } + } + if !strings.Contains(msg, "churned") { + t.Errorf("message should list the CSV columns to guide the fix; got: %q", msg) + } +} + +// TestDataDelete_EmptyArg_Friendly pins cli#76a: `data delete ""` (a positional +// arg) must give a delete-appropriate message pointing at the argument, NOT the +// shared ValidateTableName "--name" text (that flag belongs to `data ingest`). +func TestDataDelete_EmptyArg_Friendly(t *testing.T) { + var buf bytes.Buffer + err := runDataDelete(context.Background(), runDataDeleteArgs{ + Table: "", + Printer: ui.New(&buf, ui.WithColor(false)), + }) + var ee *exitError + if !errors.As(err, &ee) || ee.Code() != 2 { + t.Fatalf("err = %v, want *exitError code 2", err) + } + msg := ee.Error() + if !strings.Contains(msg, "tracebloc data delete ") { + t.Errorf("delete empty-arg message should point at the positional arg; got: %q", msg) + } + if strings.Contains(msg, "--name") { + t.Errorf("delete path must NOT point at --name; got: %q", msg) + } +} + +// TestDataIngest_KeypointNumberSetVsUnset pins cli#76b: --number-of-keypoints +// unset gets the "requires" nudge, while an explicit non-positive value gets a +// distinct "must be a positive integer (got N)" message that names the value — +// the two cases can't be told apart by the Go zero value alone. +func TestDataIngest_KeypointNumberSetVsUnset(t *testing.T) { + root := imgcLayout(t) + run := func(changed map[string]bool, n int) *exitError { + t.Helper() + var buf bytes.Buffer + err := runDataIngest(context.Background(), &buf, &buf, runDataIngestArgs{ + LocalPath: root, + Spec: push.SpecArgs{ + Table: "t", Category: "keypoint_detection", Intent: "train", + LabelColumn: "label", NumberOfKeypoints: n, + }, + ChangedFlags: changed, + Printer: ui.New(&buf, ui.WithColor(false)), + }) + var ee *exitError + if !errors.As(err, &ee) { + t.Fatalf("expected *exitError, got %v", err) + } + return ee + } + if ee := run(nil, 0); ee.Code() != 2 || !strings.Contains(ee.Error(), "requires --number-of-keypoints") { + t.Errorf("unset: got code=%d msg=%q, want exit 2 + 'requires --number-of-keypoints'", ee.Code(), ee.Error()) + } + if ee := run(map[string]bool{"number-of-keypoints": true}, 0); ee.Code() != 2 || + !strings.Contains(ee.Error(), "must be a positive integer (got 0)") { + t.Errorf("explicit 0: got code=%d msg=%q, want exit 2 + 'must be a positive integer (got 0)'", ee.Code(), ee.Error()) + } +} diff --git a/internal/cli/ingest.go b/internal/cli/ingest.go index 174ec255..b40bb58b 100644 --- a/internal/cli/ingest.go +++ b/internal/cli/ingest.go @@ -3,10 +3,12 @@ package cli import ( "fmt" "os" + "sort" "strings" "github.com/spf13/cobra" + "github.com/tracebloc/cli/internal/push" "github.com/tracebloc/cli/internal/schema" ) @@ -91,11 +93,19 @@ func runIngestValidate(cmd *cobra.Command, args []string) error { return &exitError{code: 2, err: fmt.Errorf("loading embedded schema: %w", err)} } - _, violations, parseErr := v.ValidateYAML(body) + doc, violations, parseErr := v.ValidateYAML(body) if parseErr != nil { return &exitError{code: 3, err: fmt.Errorf("%s: %w", path, parseErr)} } + // The jsonschema types every `schema` value as a bare string, so a bogus + // SQL type (e.g. {age: BANANA}) passes it — a false green that only fails + // in-cluster at CREATE TABLE. `data validate` is a local preview of what + // the cluster accepts, so mirror the ingestor's accepted-type set here too + // (cli#213), reusing the SAME push.ValidateSchemaType the --schema flag + // path uses so the two can't diverge. + violations = append(violations, schemaTypeViolations(doc)...) + if len(violations) == 0 { // Explicit discard: Fprintf returns an error when the // underlying writer fails (closed pipe, etc.). For the @@ -119,6 +129,42 @@ func runIngestValidate(cmd *cobra.Command, args []string) error { return &exitError{code: 2, err: nil} // err==nil so cobra doesn't print "Error: ..." on top } +// schemaTypeViolations previews the ingestor's accepted-SQL-type check over a +// parsed ingest doc's `schema` block (cli#213). It returns one ValidationError +// per column whose declared type the ingestor would reject, anchored at +// "schema." so it renders in the same JSON-pointer format as every +// other violation. A missing / non-map schema, or a non-string value, is left +// to the jsonschema layer — this only adds the type-vocabulary check the +// schema itself can't express. +func schemaTypeViolations(doc map[string]any) []schema.ValidationError { + if doc == nil { + return nil + } + sch, ok := doc["schema"].(map[string]any) + if !ok { + return nil + } + cols := make([]string, 0, len(sch)) + for col := range sch { + cols = append(cols, col) + } + sort.Strings(cols) // deterministic order (FormatErrors re-sorts, but keep it stable) + var out []schema.ValidationError + for _, col := range cols { + typ, ok := sch[col].(string) + if !ok { + continue + } + if err := push.ValidateSchemaType(col, typ); err != nil { + out = append(out, schema.ValidationError{ + Path: "schema." + col, + Message: err.Error(), + }) + } + } + return out +} + func plural(n int) string { if n == 1 { return "" diff --git a/internal/cli/ingest_test.go b/internal/cli/ingest_test.go index 4e7dd085..d03708ba 100644 --- a/internal/cli/ingest_test.go +++ b/internal/cli/ingest_test.go @@ -86,6 +86,66 @@ label: image_label } } +// TestIngestValidate_BogusSchemaTypeExitsTwo pins cli#213 on the `data +// validate` path: the jsonschema types every schema value as a bare string, so +// a bogus SQL type used to pass validate (false green) and only fail +// in-cluster. `data validate` now previews the ingestor's accepted-type set and +// flags it, anchored at schema.. +func TestIngestValidate_BogusSchemaTypeExitsTwo(t *testing.T) { + path := writeTmpYAML(t, ` +apiVersion: tracebloc.io/v1 +kind: IngestConfig +table: t +intent: train +category: tabular_classification +csv: /data/data.csv +schema: + age: INT + bad: BANANA +label: churned +`) + code, _, stderr := execIngestValidate(t, path) + if code != 2 { + t.Fatalf("expected exit 2 for a bogus schema type, got %d\nstderr:\n%s", code, stderr) + } + for _, want := range []string{"schema.bad", "supported SQL type"} { + if !strings.Contains(stderr, want) { + t.Errorf("expected stderr to mention %q, got:\n%s", want, stderr) + } + } + // A valid type on the same doc must NOT be flagged. + if strings.Contains(stderr, "schema.age") { + t.Errorf("a valid type (age: INT) should not be flagged; stderr:\n%s", stderr) + } +} + +// TestIngestValidate_ValidSchemaTypesOK: a doc whose types are all accepted +// (including a two-arg DECIMAL and the non-inferable DOUBLE) validates clean — +// the type preview must not over-reject. +func TestIngestValidate_ValidSchemaTypesOK(t *testing.T) { + path := writeTmpYAML(t, ` +apiVersion: tracebloc.io/v1 +kind: IngestConfig +table: t +intent: train +category: tabular_classification +schema: + age: INT + price: DECIMAL(10,2) + ratio: DOUBLE + name: VARCHAR(255) +csv: /data/data.csv +label: churned +`) + code, stdout, stderr := execIngestValidate(t, path) + if code != 0 { + t.Fatalf("expected exit 0 for all-valid types, got %d\nstderr:\n%s", code, stderr) + } + if !strings.Contains(stdout, "ok") { + t.Errorf("expected 'ok' on stdout, got: %q", stdout) + } +} + func TestIngestValidate_UnreadableFileExitsThree(t *testing.T) { // Distinct exit code (3) for file-level problems (missing, // permission-denied, etc.) — separates from schema violations diff --git a/internal/push/tabular.go b/internal/push/tabular.go index 928ac206..df99c0ec 100644 --- a/internal/push/tabular.go +++ b/internal/push/tabular.go @@ -222,15 +222,113 @@ func DiscoverTabular(rootDir string) (*LocalLayout, error) { return layout, nil } +// acceptedSQLBaseTypes is the CLI's mirror of the ONLY SQL types the ingestor +// accepts in an explicit schema — the keys of data-ingestors' +// database.py::MySQLDatabase._get_sqlalchemy_type type_mapping (di#349). That +// function is the SOURCE OF TRUTH: it upper/strip-normalizes the declared +// type, takes the base before "(", and raises ValueError("Unsupported MySQL +// type…") for anything not in this set — so a type outside it fails the +// in-cluster CREATE TABLE. Mirror it here so `data validate` / `data ingest +// --schema` reject the same tokens locally instead of a false green that only +// fails after the upload (cli#213). Keep in lock-step with the upstream map; +// do NOT narrow it to the --schema help shortlist (which lists only the +// inferable types), or the CLI would reject valid types the ingestor accepts. +var acceptedSQLBaseTypes = map[string]struct{}{ + "VARCHAR": {}, "CHAR": {}, "TEXT": {}, + "INT": {}, "INTEGER": {}, "TINYINT": {}, "SMALLINT": {}, "MEDIUMINT": {}, "BIGINT": {}, + "FLOAT": {}, "DOUBLE": {}, "DECIMAL": {}, "NUMERIC": {}, + "BOOLEAN": {}, "BOOL": {}, + "DATE": {}, "DATETIME": {}, "TIMESTAMP": {}, "TIME": {}, + "BLOB": {}, "LONGBLOB": {}, +} + +// sqlBaseType extracts the base type the ingestor keys on, byte-for-byte with +// database.py's `mysql_type.upper().strip().split("(")[0].split()[0]`: upper + +// trim, drop any "(...)" length/precision suffix, then take the FIRST +// whitespace-separated token (so "INT UNSIGNED" → "INT", matching the +// ingestor's lenient handling of trailing modifiers). Empty when the token has +// no leading identifier. +func sqlBaseType(typ string) string { + u := strings.ToUpper(strings.TrimSpace(typ)) + if i := strings.IndexByte(u, '('); i >= 0 { + u = u[:i] + } + fields := strings.Fields(u) + if len(fields) == 0 { + return "" + } + return fields[0] +} + +// ValidateSchemaType rejects a schema TYPE token whose base type isn't one the +// ingestor accepts (see acceptedSQLBaseTypes). This is a faithful mirror of a +// CONFIRMED in-cluster rejection, not a CLI-invented rule, so rejecting here is +// Principle-6-safe: the ingestor's _get_sqlalchemy_type raises on exactly these +// tokens. A recognized base with a malformed length (e.g. VARCHAR(x)) is left +// to the ingestor — its parser tolerates a bad "(…)" by dropping it — so the +// CLI only gates the base type, the part with a closed accepted set. Exported +// so BOTH the --schema flag path (ParseSchema) and the `data validate` YAML +// path (which reads a hand-authored schema block) preview the same rejection. +func ValidateSchemaType(col, typ string) error { + base := sqlBaseType(typ) + if _, ok := acceptedSQLBaseTypes[base]; ok { + return nil + } + return fmt.Errorf( + "schema type %q for column %q isn't a supported SQL type — the ingestor would "+ + "reject it in-cluster. Supported types: %s (with optional length/precision, "+ + "e.g. VARCHAR(255), DECIMAL(10,2))", + typ, col, sortedSQLTypes()) +} + +// sortedSQLTypes renders the accepted base types sorted, for error messages — +// matching the ingestor, which prints sorted(type_mapping.keys()). +func sortedSQLTypes() string { + types := make([]string, 0, len(acceptedSQLBaseTypes)) + for t := range acceptedSQLBaseTypes { + types = append(types, t) + } + sort.Strings(types) + return strings.Join(types, ", ") +} + +// splitSchemaEntries splits a --schema value on the commas that SEPARATE +// entries, ignoring commas nested inside a type's "(…)" — so a two-arg type the +// ingestor accepts, DECIMAL(10,2) / NUMERIC(p,s), stays one entry instead of +// being torn into "DECIMAL(10" + "2)". Without this the CLI rejected a type the +// cluster ingests fine (the inverse of the cli#213 false-green), so tracking +// paren depth keeps the split in lock-step with what a whole type token is. +func splitSchemaEntries(s string) []string { + var entries []string + depth, start := 0, 0 + for i, r := range s { + switch r { + case '(': + depth++ + case ')': + if depth > 0 { + depth-- + } + case ',': + if depth == 0 { + entries = append(entries, s[start:i]) + start = i + 1 + } + } + } + entries = append(entries, s[start:]) + return entries +} + // ParseSchema parses a --schema flag value of the form -// "col:TYPE,col:TYPE,..." into a column→type map. Types are passed -// through verbatim (the ingestor validates them against the SQL types -// it supports: INT, BIGINT, FLOAT, BOOLEAN, DATE, DATETIME, -// TIMESTAMP, TIME, TEXT, VARCHAR(n), ...). Whitespace around tokens -// is trimmed. +// "col:TYPE,col:TYPE,..." into a column→type map. Each TYPE token is +// validated LOCALLY against the ingestor's accepted SQL types +// (acceptedSQLBaseTypes, mirroring database.py) so a bogus type is caught +// here instead of after the upload (cli#213). Whitespace around tokens is +// trimmed. func ParseSchema(s string) (map[string]string, error) { out := map[string]string{} - for _, pair := range strings.Split(s, ",") { + for _, pair := range splitSchemaEntries(s) { pair = strings.TrimSpace(pair) if pair == "" { continue @@ -241,6 +339,9 @@ func ParseSchema(s string) (map[string]string, error) { return nil, fmt.Errorf( "schema entry %q must be col:TYPE (e.g. age:INT,price:FLOAT)", pair) } + if err := ValidateSchemaType(col, typ); err != nil { + return nil, err + } out[col] = typ } if len(out) == 0 { diff --git a/internal/push/tabular_test.go b/internal/push/tabular_test.go index ce10180f..2db9612a 100644 --- a/internal/push/tabular_test.go +++ b/internal/push/tabular_test.go @@ -5,6 +5,7 @@ import ( "bytes" "os" "path/filepath" + "strings" "testing" ) @@ -292,3 +293,77 @@ func TestParseSchema(t *testing.T) { } } } + +// TestParseSchema_TypeValidation pins cli#213: a --schema TYPE token whose base +// type isn't one the ingestor's database.py::_get_sqlalchemy_type accepts is +// rejected LOCALLY (no false green that only fails in-cluster), while every +// accepted type — including case variants, length/precision suffixes, and the +// non-inferable types the help shortlist omits — passes. +func TestParseSchema_TypeValidation(t *testing.T) { + accepted := []string{ + "a:INT", "a:integer", "a:BigInt", "a:TINYINT", "a:SMALLINT", "a:MEDIUMINT", + "a:FLOAT", "a:DOUBLE", "a:DECIMAL(10,2)", "a:NUMERIC(8,3)", + "a:BOOLEAN", "a:BOOL", "a:DATE", "a:DATETIME", "a:TIMESTAMP", "a:TIME", + "a:VARCHAR(255)", "a:char(3)", "a:TEXT", "a:BLOB", "a:LONGBLOB", + // Trailing modifier: the ingestor keys on the FIRST token, so this is accepted. + "a:INT UNSIGNED", + } + for _, ok := range accepted { + if _, err := ParseSchema(ok); err != nil { + t.Errorf("ParseSchema(%q) = %v, want accepted", ok, err) + } + } + + for _, bad := range []string{"a:BANANA", "a:STRING", "a:INTT", "a:VARCHR(10)", "a:number"} { + _, err := ParseSchema(bad) + if err == nil { + t.Errorf("ParseSchema(%q) = nil error, want a bogus-type rejection", bad) + continue + } + if !strings.Contains(err.Error(), "supported SQL type") { + t.Errorf("ParseSchema(%q) error = %q, want it to name the unsupported type", bad, err) + } + } + + // A malformed length on a RECOGNIZED base is left to the ingestor (its + // parser drops a bad "(…)"), so the CLI accepts the base rather than + // inventing a stricter rule than the ingestor (Principle 6). + if _, err := ParseSchema("a:VARCHAR(x)"); err != nil { + t.Errorf("ParseSchema(VARCHAR(x)) = %v, want accepted (base VARCHAR is valid; the ingestor tolerates a bad length)", err) + } + + // The comma inside DECIMAL(p,s) must NOT split the entry — a two-arg type + // the ingestor accepts has to survive alongside other columns. + got, err := ParseSchema("price:DECIMAL(10,2),qty:INT,name:VARCHAR(50)") + if err != nil { + t.Fatalf("ParseSchema with DECIMAL(10,2) = %v, want accepted", err) + } + want := map[string]string{"price": "DECIMAL(10,2)", "qty": "INT", "name": "VARCHAR(50)"} + for k, v := range want { + if got[k] != v { + t.Errorf("ParseSchema[%q] = %q, want %q", k, got[k], v) + } + } + if len(got) != len(want) { + t.Errorf("ParseSchema len = %d, want %d (%v)", len(got), len(want), got) + } +} + +// TestSQLBaseType mirrors database.py's base extraction +// (upper().strip().split("(")[0].split()[0]) so the accepted-type gate keys on +// exactly what the ingestor keys on. +func TestSQLBaseType(t *testing.T) { + cases := map[string]string{ + "int": "INT", + " VarChar(255)": "VARCHAR", + "DECIMAL(10, 2)": "DECIMAL", + "INT UNSIGNED": "INT", + "": "", + " ": "", + } + for in, want := range cases { + if got := sqlBaseType(in); got != want { + t.Errorf("sqlBaseType(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/schema/validate.go b/internal/schema/validate.go index b71266e8..d3662bff 100644 --- a/internal/schema/validate.go +++ b/internal/schema/validate.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "regexp" "sort" "strings" @@ -166,7 +167,7 @@ func (v *Validator) ValidateYAML(input []byte) (parsed map[string]any, errs []Va // of the validator's traversal path. var ve *jsonschema.ValidationError if errors_as(err, &ve) { - errs = flattenValidationError(ve) + errs = suppressOneOfTypeNoise(flattenValidationError(ve)) } else { // Defensive: shouldn't happen with current jsonschema/v6, // but fall back to the raw error string rather than @@ -178,6 +179,50 @@ func (v *Validator) ValidateYAML(input []byte) (parsed map[string]any, errs []Va return asMap, errs, nil } +// oneOfTypeMismatchRE matches the leaf message jsonschema emits when a oneOf +// (or anyOf) alternative fails purely on type — e.g. "got object, want string" +// / "got string, want object". These fire at the PARENT path (the `label` +// node) while the alternative the user actually meant fails deeper (at +// `label.policy`), so the parent line is noise once a more specific sibling +// exists. +var oneOfTypeMismatchRE = regexp.MustCompile(`^got \w+, want \w+$`) + +// suppressOneOfTypeNoise drops a parent-level "wrong type" error when a MORE +// SPECIFIC error exists under the same path (#76c). Concretely: a bad +// --label-policy makes the schema report both `label: got object, want string` +// (the string-alternative of the label oneOf failing by type) and +// `label.policy: value must be one of …` (the real problem); the first is +// internal validator noise that confuses the customer. An error is suppressed +// only when BOTH hold, so genuinely-different errors survive: +// - its message is a bare oneOf/anyOf type mismatch (oneOfTypeMismatchRE), and +// - another error's path is strictly nested under it (path == P, other == P+".…"). +// +// A lone `label: got string, want object` (e.g. a regression task given a +// string label, with no deeper error) has no nested sibling and is kept — so +// the "object form required" diagnostic still surfaces. +func suppressOneOfTypeNoise(errs []ValidationError) []ValidationError { + if len(errs) < 2 { + return errs + } + hasNestedUnder := func(path string) bool { + prefix := path + "." + for _, other := range errs { + if other.Path != path && strings.HasPrefix(other.Path, prefix) { + return true + } + } + return false + } + out := make([]ValidationError, 0, len(errs)) + for _, e := range errs { + if oneOfTypeMismatchRE.MatchString(e.Message) && hasNestedUnder(e.Path) { + continue + } + out = append(out, e) + } + return out +} + // flattenValidationError walks the jsonschema error tree to produce // one leaf error per real violation. The library returns a tree // because oneOf / anyOf / allOf can fail in multiple ways at once; diff --git a/internal/schema/validate_test.go b/internal/schema/validate_test.go index c09ae593..8c3b4cf6 100644 --- a/internal/schema/validate_test.go +++ b/internal/schema/validate_test.go @@ -268,6 +268,104 @@ label: churned } } +// TestValidate_SuppressesOneOfTypeNoise pins cli#76c: a bad --label-policy +// makes the label oneOf report BOTH the real "label.policy: value must be one +// of …" and the internal "label: got object, want string" (the string +// alternative failing by type). The parent-level type line is noise once the +// deeper, specific error exists, so ValidateYAML drops it — but a lone parent +// type mismatch (regression given a string label, no deeper error) is kept. +func TestValidate_SuppressesOneOfTypeNoise(t *testing.T) { + v := mustValidator(t) + + // Bad policy: the specific label.policy error survives; the sibling + // "label: got object, want string" noise is suppressed. + _, errs, perr := v.ValidateYAML([]byte(` +apiVersion: tracebloc.io/v1 +kind: IngestConfig +table: t +intent: train +category: tabular_regression +csv: /data/houses.csv +schema: + price: FLOAT +label: + column: price + policy: nonsense +`)) + if perr != nil { + t.Fatalf("parse: %v", perr) + } + var sawPolicy, sawNoise bool + for _, e := range errs { + if e.Path == "label.policy" { + sawPolicy = true + } + if e.Path == "label" && oneOfTypeMismatchRE.MatchString(e.Message) { + sawNoise = true + } + } + if !sawPolicy { + t.Errorf("expected the specific label.policy error to survive; got:\n%s", FormatErrors(errs)) + } + if sawNoise { + t.Errorf("the parent-level 'label: got object, want string' noise should be suppressed; got:\n%s", FormatErrors(errs)) + } + + // A regression task given a plain string label has ONLY the parent type + // mismatch (no deeper error) — it must be KEPT so "object form required" + // still surfaces. + _, errs2, perr := v.ValidateYAML([]byte(` +apiVersion: tracebloc.io/v1 +kind: IngestConfig +table: t +intent: train +category: tabular_regression +csv: /data/houses.csv +schema: + price: FLOAT +label: price +`)) + if perr != nil { + t.Fatalf("parse: %v", perr) + } + keptLabel := false + for _, e := range errs2 { + if e.Path == "label" { + keptLabel = true + } + } + if !keptLabel { + t.Errorf("a lone parent-level label type mismatch must be kept; got:\n%s", FormatErrors(errs2)) + } +} + +// TestSuppressOneOfTypeNoise unit-tests the filter directly, decoupled from the +// live schema, so the prefix/regex rule is pinned independently. +func TestSuppressOneOfTypeNoise(t *testing.T) { + in := []ValidationError{ + {Path: "label", Message: "got object, want string"}, + {Path: "label.policy", Message: "value must be one of 'passthrough', 'bucket'"}, + {Path: "table", Message: "got number, want string"}, // no child → kept + } + out := suppressOneOfTypeNoise(in) + for _, e := range out { + if e.Path == "label" && oneOfTypeMismatchRE.MatchString(e.Message) { + t.Errorf("label noise should be dropped (label.policy present); got %+v", out) + } + } + var sawTable, sawPolicy bool + for _, e := range out { + sawTable = sawTable || e.Path == "table" + sawPolicy = sawPolicy || e.Path == "label.policy" + } + if !sawTable { + t.Errorf("a type mismatch with NO nested sibling must be kept; got %+v", out) + } + if !sawPolicy { + t.Errorf("the specific nested error must be kept; got %+v", out) + } +} + // Parse-level failures (vs schema violations) need a separate // failure mode so callers can render them differently in the UI — // "your file isn't YAML" vs "your file is YAML but doesn't match