From b9b0fa352c303e493c41eacfed6d6a474e709802 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 21 Jul 2026 13:12:15 +0200 Subject: [PATCH] fix(ingest): mirror semseg-mask + missing DataValidator parity in preflight (cli#352) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `data ingest` preflight under-mirrored two in-cluster validators, so some datasets ACCEPTED locally then REJECTED after the full upload: - Semantic-segmentation mask resolution: the ingestor runs a SECOND ImageResolutionValidator ("Mask Resolution Validator", subdir=masks) the CLI never replicated, so a corrupt or mis-sized mask slipped through. Now previewed by ValidateMaskResolution over masks/, with the same target-size + min-size as the images (shared decode core with ValidateImages). - DataValidator per-value NUMERIC type check: for tabular / time-series schemas the ingestor validates every value against its declared type; the CLI only checked column presence. Now previewed by CheckColumnValueTypes: a non-numeric or fractional value in an INT column, or a non-numeric value in a FLOAT column, is caught locally. Scope is deliberately numeric-only and never-over-rejects (string / boolean / date length + coercion, integer overflow and non-finite stay a documented under-preview — mirroring pandas there would risk rejecting values the cluster accepts). Parity corpus extended with 4 cases (semseg mask-res mismatch, INT non-numeric, INT non-integer, and a fully-typed accept guard). Pin left unchanged: both validators already exist at the pinned data-ingestors ref, so the drift is a mirror gap, not a stale pin. Co-Authored-By: Claude Opus 4.8 --- internal/push/image_resolution.go | 93 +++++++ internal/push/image_resolution_test.go | 68 +++++ internal/push/preflight.go | 51 ++-- internal/push/testdata/parity/cases.json | 54 ++++ .../semseg-mask-res-mismatch/images/a.jpg | Bin 0 -> 644 bytes .../semseg-mask-res-mismatch/images/b.jpg | Bin 0 -> 644 bytes .../cases/semseg-mask-res-mismatch/labels.csv | 3 + .../semseg-mask-res-mismatch/masks/a_mask.png | Bin 0 -> 88 bytes .../semseg-mask-res-mismatch/masks/b_mask.png | Bin 0 -> 202 bytes .../cases/tabular-int-noninteger/data.csv | 3 + .../cases/tabular-int-nonnumeric/data.csv | 3 + .../parity/cases/tabular-typed-ok/data.csv | 3 + internal/push/testdata/parity/goldens.json | 24 ++ internal/push/value_types.go | 239 ++++++++++++++++++ internal/push/value_types_test.go | 207 +++++++++++++++ 15 files changed, 720 insertions(+), 28 deletions(-) create mode 100644 internal/push/image_resolution.go create mode 100644 internal/push/image_resolution_test.go create mode 100644 internal/push/testdata/parity/cases/semseg-mask-res-mismatch/images/a.jpg create mode 100644 internal/push/testdata/parity/cases/semseg-mask-res-mismatch/images/b.jpg create mode 100644 internal/push/testdata/parity/cases/semseg-mask-res-mismatch/labels.csv create mode 100644 internal/push/testdata/parity/cases/semseg-mask-res-mismatch/masks/a_mask.png create mode 100644 internal/push/testdata/parity/cases/semseg-mask-res-mismatch/masks/b_mask.png create mode 100644 internal/push/testdata/parity/cases/tabular-int-noninteger/data.csv create mode 100644 internal/push/testdata/parity/cases/tabular-int-nonnumeric/data.csv create mode 100644 internal/push/testdata/parity/cases/tabular-typed-ok/data.csv create mode 100644 internal/push/value_types.go create mode 100644 internal/push/value_types_test.go diff --git a/internal/push/image_resolution.go b/internal/push/image_resolution.go new file mode 100644 index 00000000..e33d8af8 --- /dev/null +++ b/internal/push/image_resolution.go @@ -0,0 +1,93 @@ +package push + +import ( + "fmt" + "image" + "os" + "path/filepath" +) + +// scanImageResolutions decodes each file's header (cheap, no full decode) and +// sorts it into three buckets — the shared core of the two ImageResolution +// previews: ValidateImages (the images/ dir) and ValidateMaskResolution (the +// masks/ dir). Keeping ONE decode-and-compare here is what lets the CLI mirror +// the ingestor's TWO ImageResolutionValidator instances for semantic +// segmentation (modalities/validators.py) without the rule drifting between +// them. +// +// - broken: zero-byte, unreadable, or undecodable files; +// - tooSmall: below the minW×minH floor (EITHER side under), when a floor is +// set — mirrors _meets_min_size; +// - mismatched: resolution != expectedW×expectedH (exact, no resize), when an +// expected size is set. +// +// expectedW/H or minW/H of 0 disables that comparison, exactly as documented on +// ValidateImages (see it for the parity rationale). Each offender string carries +// the file name and its dimensions for the caller's message. +func scanImageResolutions(paths []string, expectedW, expectedH, minW, minH int) (broken, tooSmall, mismatched []string) { + for _, path := range paths { + name := filepath.Base(path) + f, err := os.Open(path) + if err != nil { + broken = append(broken, fmt.Sprintf("%s (unreadable: %v)", name, err)) + continue + } + cfg, _, err := image.DecodeConfig(f) + _ = f.Close() + if err != nil { + if st, serr := os.Stat(path); serr == nil && st.Size() == 0 { + broken = append(broken, name+" (empty file, 0 bytes)") + } else { + broken = append(broken, name+" (not a valid image — corrupt or unsupported format)") + } + continue + } + if minW > 0 && minH > 0 && (cfg.Width < minW || cfg.Height < minH) { + tooSmall = append(tooSmall, fmt.Sprintf("%s (%dx%d)", name, cfg.Width, cfg.Height)) + } + if expectedW > 0 && expectedH > 0 && (cfg.Width != expectedW || cfg.Height != expectedH) { + mismatched = append(mismatched, fmt.Sprintf("%s (%dx%d)", name, cfg.Width, cfg.Height)) + } + } + return broken, tooSmall, mismatched +} + +// ValidateMaskResolution previews the ingestor's SECOND ImageResolutionValidator +// for semantic_segmentation — the one named "Mask Resolution Validator" with +// subdir="masks" (modalities/validators.py semantic_segmentation): it reads +// every PNG mask and rejects zero-byte / undecodable files, masks below the +// minimum-size floor, and any mask whose resolution differs from the expected +// target size. Masks are pixel-wise label maps, so they must share the images' +// resolution — the ingestor constructs this validator with the SAME +// expected_resolution (target_size) and min_size as the images' +// ImageResolutionValidator, and this preview is called with those same values. +// +// Before cli#352 the CLI validated only images/ resolution (ValidateImages) and +// never the masks, so a corrupt or mis-sized mask passed local preflight and +// then failed in-cluster after the full upload — the accept-then-reject the +// parity contract exists to prevent. The too-small floor takes precedence over +// the resolution mismatch, matching ValidateImages and the ingestor's ordering. +func ValidateMaskResolution(masks []string, expectedW, expectedH, minW, minH int) error { + const maxListed = 5 + broken, tooSmall, mismatched := scanImageResolutions(masks, expectedW, expectedH, minW, minH) + if len(tooSmall) > 0 { + return fmt.Errorf( + "%d mask(s) are smaller than the %dx%d minimum you set with --min-size: %s. "+ + "Provide larger masks, or lower the floor with --min-size, then re-run.", + len(tooSmall), minW, minH, TruncateList(tooSmall, maxListed)) + } + if len(broken) > 0 { + return fmt.Errorf( + "%d mask(s) in masks/ can't be ingested: %s. The cluster reads every mask as a PNG "+ + "and rejects these after the upload — fix or remove them and re-run.", + len(broken), TruncateList(broken, maxListed)) + } + if len(mismatched) > 0 { + return fmt.Errorf( + "%d mask(s) don't match the %dx%d resolution the images use: %s. Semantic-segmentation "+ + "masks are pixel-wise label maps, so each mask must be exactly the image size — the "+ + "cluster validates this after the upload. Resize the masks to match and re-run.", + len(mismatched), expectedW, expectedH, TruncateList(mismatched, maxListed)) + } + return nil +} diff --git a/internal/push/image_resolution_test.go b/internal/push/image_resolution_test.go new file mode 100644 index 00000000..256c79c4 --- /dev/null +++ b/internal/push/image_resolution_test.go @@ -0,0 +1,68 @@ +package push + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestValidateMaskResolution mirrors TestValidateImages for the semseg "Mask +// Resolution Validator" preview (cli#352): masks at the target size pass; a +// mask whose resolution differs is rejected (naming it and both sizes, which +// proves the dimensions were decoded — not merely that the file is unreadable); +// a zero-byte / corrupt mask is rejected; the min-size floor applies; and an +// empty set or a 0-expected size is a no-op. +func TestValidateMaskResolution(t *testing.T) { + dir := t.TempDir() + write := func(name string, body []byte) string { + p := filepath.Join(dir, name) + if err := os.WriteFile(p, body, 0o644); err != nil { + t.Fatal(err) + } + return p + } + good := write("a_mask.png", pngBytes(t, 32, 32)) + wrong := write("b_mask.png", pngBytes(t, 48, 48)) + zero := write("z_mask.png", nil) + + if err := ValidateMaskResolution(nil, 32, 32, 0, 0); err != nil { + t.Errorf("empty mask set must pass: %v", err) + } + if err := ValidateMaskResolution([]string{good}, 32, 32, 0, 0); err != nil { + t.Errorf("32x32 mask against a 32x32 target rejected: %v", err) + } + + // Wrong resolution: rejected, naming the file and BOTH sizes — the decode + // happened, so this is a genuine size mismatch, not a broken-file fallback. + err := ValidateMaskResolution([]string{good, wrong}, 32, 32, 0, 0) + if err == nil { + t.Fatal("48x48 mask against a 32x32 target must be rejected (cli#352)") + } + for _, want := range []string{"b_mask.png", "48x48", "32x32", "resolution"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("mismatch error missing %q: %v", want, err) + } + } + + // Zero-byte mask can't be ingested. + if err := ValidateMaskResolution([]string{good, zero}, 32, 32, 0, 0); err == nil { + t.Fatal("zero-byte mask must be rejected") + } else if !strings.Contains(err.Error(), "0 bytes") { + t.Errorf("zero-byte diagnosis missing: %v", err) + } + + // 0-expected size skips the resolution comparison (auto-detect path). + if err := ValidateMaskResolution([]string{good, wrong}, 0, 0, 0, 0); err != nil { + t.Errorf("no expected size → no resolution rejection: %v", err) + } + + // Min-size floor applies to masks too (same as images): a below-floor mask + // is rejected even with no target size. + tiny := write("tiny_mask.png", pngBytes(t, 16, 16)) + if err := ValidateMaskResolution([]string{tiny}, 0, 0, 32, 32); err == nil { + t.Fatal("below-floor mask must be rejected") + } else if !strings.Contains(err.Error(), "minimum") { + t.Errorf("floor diagnosis missing: %v", err) + } +} diff --git a/internal/push/preflight.go b/internal/push/preflight.go index bfb9c5ea..3aed4a43 100644 --- a/internal/push/preflight.go +++ b/internal/push/preflight.go @@ -18,7 +18,6 @@ import ( "encoding/csv" "errors" "fmt" - "image" "io" "math" "os" @@ -243,33 +242,10 @@ func CheckHasDataRows(path string) error { // target_size uniformity error. func ValidateImages(images []string, expectedW, expectedH, minW, minH int) error { const maxListed = 5 - var broken, tooSmall, mismatched []string - for _, path := range images { - name := filepath.Base(path) - f, err := os.Open(path) - if err != nil { - broken = append(broken, fmt.Sprintf("%s (unreadable: %v)", name, err)) - continue - } - cfg, _, err := image.DecodeConfig(f) - _ = f.Close() - if err != nil { - if st, serr := os.Stat(path); serr == nil && st.Size() == 0 { - broken = append(broken, name+" (empty file, 0 bytes)") - } else { - broken = append(broken, name+" (not a valid image — corrupt or unsupported format)") - } - continue - } - if minW > 0 && minH > 0 && (cfg.Width < minW || cfg.Height < minH) { - tooSmall = append(tooSmall, - fmt.Sprintf("%s (%dx%d)", name, cfg.Width, cfg.Height)) - } - if expectedW > 0 && expectedH > 0 && (cfg.Width != expectedW || cfg.Height != expectedH) { - mismatched = append(mismatched, - fmt.Sprintf("%s (%dx%d)", name, cfg.Width, cfg.Height)) - } - } + // scanImageResolutions (image_resolution.go) is the shared decode-and-compare + // core — the SAME one ValidateMaskResolution runs over masks/, so the two + // ImageResolutionValidator previews can't drift (cli#352). + broken, tooSmall, mismatched := scanImageResolutions(images, expectedW, expectedH, minW, minH) // Floor first: an image below the minimum size simply can't be trained // on, so it's the most fundamental, actionable failure — data-ingestors // #348 returns it ahead of the uniformity / target_size mismatch. @@ -1444,6 +1420,16 @@ func PreflightDataset(spec SpecArgs, layout *LocalLayout) (notes []string, probl if err := CheckSchemaColumns(header, spec.Schema, "the data CSV"); err != nil { return nil, dataProblem(err) } + // Per-value TYPE check (DataValidator preview, cli#352): a value that + // doesn't match its column's declared NUMERIC type — a non-numeric or + // fractional value in an INT column, a non-numeric value in a FLOAT + // column — is rejected by the ingestor's DataValidator after the table + // is created; CheckSchemaColumns only proves the columns EXIST. See + // CheckColumnValueTypes for the deliberately-narrow, never-over-reject + // scope (numeric types only). + if err := CheckColumnValueTypes(layout.LabelsCSV, spec.Schema, spec.Category); err != nil { + return nil, dataProblem(err) + } // A bogus --label-column otherwise fails in-cluster only at READ // time — after the table was created — leaving an orphaned table. if err := CheckLabelColumn(header, spec.LabelColumn, "the data CSV"); err != nil { @@ -1594,6 +1580,15 @@ func PreflightDataset(spec SpecArgs, layout *LocalLayout) (notes []string, probl if err := CheckMaskIDColumn(layout.LabelsCSV); err != nil { return nil, dataProblem(err) } + // Mask resolution — the ingestor's SECOND ImageResolutionValidator + // ("Mask Resolution Validator", subdir=masks). Masks are pixel-wise + // label maps and must share the images' target size + min-size floor; + // a corrupt or mis-sized mask otherwise passes preflight and fails + // in-cluster after the upload (cli#352). Same expected/floor as the + // images (expW/expH/minW/minH), mirroring the factory. + if err := ValidateMaskResolution(layout.Sidecars["masks"], expW, expH, minW, minH); err != nil { + return nil, dataProblem(err) + } missing, orphanFiles, cerr := CrossCheckLabels(layout.LabelsCSV, layout.Images, spec.Extension) if cerr != nil { return nil, dataProblem(cerr) diff --git a/internal/push/testdata/parity/cases.json b/internal/push/testdata/parity/cases.json index 60540f02..1ff149ae 100644 --- a/internal/push/testdata/parity/cases.json +++ b/internal/push/testdata/parity/cases.json @@ -841,6 +841,60 @@ "cli_verdict": "accept", "ingestor_verdict": "reject", "note": "DOCUMENTED under-preview: a negative 'time' value \u2014 TimeToEventValidator rejects in-cluster, but the CLI has no TTE time-column mirror, so the failure surfaces post-upload. Candidate for a future TTE preview." + }, + { + "extension": ".jpg", + "target_size": [ + 32, + 32 + ], + "csv": "labels.csv", + "name": "semseg-mask-res-mismatch", + "category": "semantic_segmentation", + "label_column": "label", + "cli_verdict": "reject", + "ingestor_verdict": "reject", + "note": "b_mask.png is 48x48 while the images (and target_size) are 32x32: the ingestor's SECOND ImageResolutionValidator (name 'Mask Resolution Validator', subdir 'masks') rejects it, and the CLI now previews it via ValidateMaskResolution (cli#352). Pairing + mask_id are valid, so mask resolution is the sole rejecter. Before cli#352 the CLI validated only images/ and accepted this, then it failed in-cluster after the upload." + }, + { + "csv": "data.csv", + "name": "tabular-int-nonnumeric", + "category": "tabular_classification", + "label_column": "label", + "schema": { + "age": "INT", + "label": "VARCHAR(255)" + }, + "cli_verdict": "reject", + "ingestor_verdict": "reject", + "note": "age is declared INT but data row 1 is 'abc': the ingestor's DataValidator rejects the non-numeric value (labels cat/dog keep diversity happy, so DataValidator is the sole rejecter), and the CLI now previews it via CheckColumnValueTypes (cli#352). Explicit schema so BOTH sides validate the same typed column." + }, + { + "csv": "data.csv", + "name": "tabular-int-noninteger", + "category": "tabular_classification", + "label_column": "label", + "schema": { + "age": "INT", + "label": "VARCHAR(255)" + }, + "cli_verdict": "reject", + "ingestor_verdict": "reject", + "note": "age is declared INT but data row 2 is '40.5': DataValidator's fractional-value check rejects it; CheckColumnValueTypes previews the same (cli#352). Pins the non-integer sub-check as distinct from the non-numeric one above." + }, + { + "csv": "data.csv", + "name": "tabular-typed-ok", + "category": "tabular_classification", + "label_column": "label", + "schema": { + "age": "INT", + "price": "FLOAT", + "label": "VARCHAR(255)" + }, + "cli_verdict": "accept", + "ingestor_verdict": "accept", + "note": "OVER-REJECT guard for cli#352: a fully-typed schema whose values match (INT age, FLOAT price incl. '1e3' scientific notation, VARCHAR label with 2 classes) must pass on BOTH sides. Proves CheckColumnValueTypes accepts numeric-looking values (the #188 direction) rather than over-rejecting a valid typed dataset." } ] } diff --git a/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/images/a.jpg b/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/images/a.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6520d3907647976516c4c9c3aaddbec125af8aef GIT binary patch literal 644 zcmex=``2_j6xdp@o1cgOJMMZh|#U;coyG6VInuyV4pa*FVB^NNrR z{vTivP+(?MVqg+vWEN!ne}q9E=uTEfFkpZJMkb&e+1NQaxwwG}whAyXF)}kV zu`q*N4OCqVlxJWOWEE00bYv3_Ok`Io6ftU?xR68HY2!iBpoX!XqN1l2cOC(lau%ic3n%$}1|Xnp;}i+B-VC zCQY6)b=ve9GiNPYykzOJeA&aSFc^aar4&0 zM~|O8efIpt%U2&ieg5+G+xH(oe}VkP$iNKo7LbH^49#DHKz}i@urRZ*gZ#zFR1U<< zf-J0xhHOHPf$WKe!b(Ps93oB=7j8VrscandK{To8BA1wo$wSqTAg_UaMx4i*$nqK7 lV+eoUV&GwB1V$dSAcH-_^B0S{KJr~y)TO}y#mxV20s#Es$tVB- literal 0 HcmV?d00001 diff --git a/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/images/b.jpg b/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/images/b.jpg new file mode 100644 index 0000000000000000000000000000000000000000..6520d3907647976516c4c9c3aaddbec125af8aef GIT binary patch literal 644 zcmex=``2_j6xdp@o1cgOJMMZh|#U;coyG6VInuyV4pa*FVB^NNrR z{vTivP+(?MVqg+vWEN!ne}q9E=uTEfFkpZJMkb&e+1NQaxwwG}whAyXF)}kV zu`q*N4OCqVlxJWOWEE00bYv3_Ok`Io6ftU?xR68HY2!iBpoX!XqN1l2cOC(lau%ic3n%$}1|Xnp;}i+B-VC zCQY6)b=ve9GiNPYykzOJeA&aSFc^aar4&0 zM~|O8efIpt%U2&ieg5+G+xH(oe}VkP$iNKo7LbH^49#DHKz}i@urRZ*gZ#zFR1U<< zf-J0xhHOHPf$WKe!b(Ps93oB=7j8VrscandK{To8BA1wo$wSqTAg_UaMx4i*$nqK7 lV+eoUV&GwB1V$dSAcH-_^B0S{KJr~y)TO}y#mxV20s#Es$tVB- literal 0 HcmV?d00001 diff --git a/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/labels.csv b/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/labels.csv new file mode 100644 index 00000000..69325d5a --- /dev/null +++ b/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/labels.csv @@ -0,0 +1,3 @@ +filename,mask_id +a.jpg,a_mask.png +b.jpg,b_mask.png diff --git a/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/masks/a_mask.png b/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/masks/a_mask.png new file mode 100644 index 0000000000000000000000000000000000000000..3c5f2434505699cf73450b49e9d24fc84c93f07c GIT binary patch literal 88 zcmeAS@N?(olHy`uVBq!ia0vp^3LwnL3?x0byx0z;m;-!5T!HlRD%)E?O2E^_F{ENn i@{j-X8ygcp2r=mP*dI)7oHiFI$KdJe=d#Wzp$Py@n-`V< literal 0 HcmV?d00001 diff --git a/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/masks/b_mask.png b/internal/push/testdata/parity/cases/semseg-mask-res-mismatch/masks/b_mask.png new file mode 100644 index 0000000000000000000000000000000000000000..2aadadaf44b5fecca0fc1795f26394a889caec44 GIT binary patch literal 202 zcmeAS@N?(olHy`uVBq!ia0vp^1|ZDA1SD@HSpG?CK;nT0MrJl134;Vo#)I+%X#)nv?W#o}H!yg*`njxgN@xNAM%N}l literal 0 HcmV?d00001 diff --git a/internal/push/testdata/parity/cases/tabular-int-noninteger/data.csv b/internal/push/testdata/parity/cases/tabular-int-noninteger/data.csv new file mode 100644 index 00000000..db72d877 --- /dev/null +++ b/internal/push/testdata/parity/cases/tabular-int-noninteger/data.csv @@ -0,0 +1,3 @@ +age,label +30,cat +40.5,dog diff --git a/internal/push/testdata/parity/cases/tabular-int-nonnumeric/data.csv b/internal/push/testdata/parity/cases/tabular-int-nonnumeric/data.csv new file mode 100644 index 00000000..4d6ed6bc --- /dev/null +++ b/internal/push/testdata/parity/cases/tabular-int-nonnumeric/data.csv @@ -0,0 +1,3 @@ +age,label +abc,cat +40,dog diff --git a/internal/push/testdata/parity/cases/tabular-typed-ok/data.csv b/internal/push/testdata/parity/cases/tabular-typed-ok/data.csv new file mode 100644 index 00000000..c75ef4e7 --- /dev/null +++ b/internal/push/testdata/parity/cases/tabular-typed-ok/data.csv @@ -0,0 +1,3 @@ +age,price,label +30,1e3,cat +40,2.5,dog diff --git a/internal/push/testdata/parity/goldens.json b/internal/push/testdata/parity/goldens.json index 0446d06b..3b8920bf 100644 --- a/internal/push/testdata/parity/goldens.json +++ b/internal/push/testdata/parity/goldens.json @@ -216,6 +216,14 @@ ], "verdict": "reject" }, + "semseg-mask-res-mismatch": { + "errors": [ + "ImageResolutionValidator: Multiple image resolutions found: [(32, 32), (48, 48)]. All images must have the same resolution.", + "ImageResolutionValidator: Expected resolution: [32, 32]", + "ImageResolutionValidator: Resolution errors: ['masks/b_mask.png: (48, 48) (expected: [32, 32])']" + ], + "verdict": "reject" + }, "semseg-missing-mask": { "errors": [ "FilePairingValidator: 1 image(s) have no matching mask: ['b']" @@ -295,6 +303,18 @@ ], "verdict": "reject" }, + "tabular-int-noninteger": { + "errors": [ + "DataValidator: Column 'age' contains 1 non-integer values" + ], + "verdict": "reject" + }, + "tabular-int-nonnumeric": { + "errors": [ + "DataValidator: Column 'age' contains 1 non-numeric value(s) at row 0." + ], + "verdict": "reject" + }, "tabular-label-missing": { "errors": [], "verdict": "accept" @@ -325,6 +345,10 @@ }, "verdict": "accept" }, + "tabular-typed-ok": { + "errors": [], + "verdict": "accept" + }, "tabular-varchar-numeric-labels": { "errors": [], "values": { diff --git a/internal/push/value_types.go b/internal/push/value_types.go new file mode 100644 index 00000000..0007aeb9 --- /dev/null +++ b/internal/push/value_types.go @@ -0,0 +1,239 @@ +package push + +import ( + "errors" + "fmt" + "io" + "math" + "path/filepath" + "strconv" + "strings" +) + +// isIntegerSQLType / isFloatSQLType classify a schema base type (as produced by +// sqlBaseType) into the two numeric families whose per-value contract +// CheckColumnValueTypes previews. They mirror DataValidator.type_validators +// (validators/data_validator.py): the INT family shares one integer contract +// (parseable AND whole), the FLOAT family (incl. DECIMAL/NUMERIC) one +// real-number contract. +func isIntegerSQLType(base string) bool { + switch base { + case "INT", "INTEGER", "TINYINT", "SMALLINT", "MEDIUMINT", "BIGINT": + return true + } + return false +} + +func isFloatSQLType(base string) bool { + switch base { + case "FLOAT", "DOUBLE", "DECIMAL", "NUMERIC": + return true + } + return false +} + +// CheckColumnValueTypes previews the NUMERIC portion of the ingestor's +// DataValidator (validators/data_validator.py), composed for every tabular / +// time-series category that carries a schema (modalities/validators.py). +// DataValidator scans the FULL column and rejects a value that does not match +// its declared SQL type BEFORE the table is created; the CLI otherwise only +// proves the schema's COLUMNS exist (CheckSchemaColumns), so a schema/value TYPE +// mismatch — a non-numeric string in an INT/FLOAT column, or a fractional value +// in an INT column — passed preflight and then rejected in-cluster after the +// upload, orphaning a freshly-created empty table (cli#352). The gap bites an +// explicit --schema that disagrees with the data AND an inferred schema whose +// bad value first appears past the inference sample (InferSchema samples 5000 +// rows; the ingestor scans them all). +// +// SCOPE — numeric types only, and only the SAFE (never-over-reject) subset: +// - INT family (INT/INTEGER/TINYINT/SMALLINT/MEDIUMINT/BIGINT): a present, +// non-NA cell that isn't a finite number, or that has a fractional part +// (the ingestor's `to_numeric` + `numeric % 1 != 0`); +// - FLOAT family (FLOAT/DOUBLE/DECIMAL/NUMERIC): a present, non-NA cell that +// isn't a number. +// +// The string (VARCHAR/CHAR/TEXT length), BOOLEAN, and DATE/DATETIME/TIMESTAMP/ +// TIME per-value checks are DELIBERATELY not mirrored, and integer OVERFLOW and +// non-finite (inf) are left to the ingestor: reproducing pandas' to_datetime / +// boolean-vocabulary / length / range coercion in Go risks REJECTING a value +// the cluster accepts — the dangerous direction (a burned upload), the same +// reason perGroupTimeViolation leaves the date branch an under-preview. Those +// stay a documented under-preview; the CLI only ever UNDER-rejects here, never +// over-rejects (the parity contract's cardinal rule). +// +// Value read (mirrors how the ingestor reads a TYPED numeric column: +// na_values=coercion.build_csv_na_values(schema) — which is NA_SENTINELS for +// every schema column — with keep_default_na=False): a cell is "present" iff its +// RAW cell is not an NA sentinel (matched on the raw cell, since pandas tokenises +// NA before stripping) and is not whitespace-only. A missing value is stored +// NULL, never flagged — the ingestor's `numeric.isna() & series.notna()` mask. +// Numbers are parsed after trimming (pd.to_numeric tolerates surrounding +// whitespace); ParseFloat accepts a superset of pandas' numeric grammar for real +// inputs, so a ParseFloat failure is a value pandas also rejects (no over-reject). +// +// Columns are resolved by their STRIPPED header name, case-SENSITIVELY, exactly +// as DataValidator matches schema keys after `columns.str.strip()`. The +// category's excluded time column (time_series_classification's grouping time +// column, checked by PerGroupTimeOrderedValidator — which accepts a fractional +// step index as a valid position, not by DataValidator) is skipped so a +// fractional index the cluster ingests fine isn't over-rejected here. +func CheckColumnValueTypes(csvPath string, schema map[string]string, category string) error { + if len(schema) == 0 { + return nil + } + // The time column the category's DataValidator composition drops (mirrors + // modalities/validators.py): time_series_classification hands DataValidator + // the schema MINUS its grouping time column. A time_series_forecasting + // "timestamp" is TIMESTAMP-typed and is already skipped as a non-numeric + // type, so it needs no separate name rule here. + excludedTime := "" + if g, grouped := GroupingFor(category); grouped { + excludedTime = strings.TrimSpace(g.TimeColumn) + } + // Which schema columns carry a numeric per-value contract, keyed by their + // stripped name (matching the header resolution below). + intCols := map[string]bool{} + floatCols := map[string]bool{} + for col, typ := range schema { + name := strings.TrimSpace(col) + if name == excludedTime { + continue + } + switch base := sqlBaseType(typ); { + case isIntegerSQLType(base): + intCols[name] = true + case isFloatSQLType(base): + floatCols[name] = true + } + } + if len(intCols) == 0 && len(floatCols) == 0 { + return nil // no numeric-typed columns — nothing this preview covers + } + + r, closer, err := openCSVReader(csvPath) + if err != nil { + return nil // unreadable file is another check's diagnostic (CheckCSVEncoding) + } + defer func() { _ = closer.Close() }() + header, err := r.Read() + if err != nil { + return nil // no header — another check's diagnostic + } + + // Resolve each numeric column to its header index by STRIPPED name, + // case-SENSITIVELY — how DataValidator matches schema keys after + // columns.str.strip(). First occurrence wins; a duplicate header is + // CheckDuplicateHeaders' diagnostic, not this one's. + type colCheck struct { + name string + idx int + integer bool + } + var checks []colCheck + seenHdr := map[string]bool{} + for i, h := range header { + name := strings.TrimSpace(h) + if seenHdr[name] { + continue + } + seenHdr[name] = true + switch { + case intCols[name]: + checks = append(checks, colCheck{name, i, true}) + case floatCols[name]: + checks = append(checks, colCheck{name, i, false}) + } + } + if len(checks) == 0 { + return nil // schema columns absent from the header is CheckSchemaColumns' diagnostic + } + + const maxSample = 5 + type offense struct { + integer bool + count int + rows []string + } + offenses := map[string]*offense{} + row := 0 + for { + rec, rerr := r.Read() + if errors.Is(rerr, io.EOF) { + break + } + if rerr != nil { + // Mid-read failure: the unread tail could hide (or clear) offenders. + // Fail closed rather than pass a partial scan (matches + // CrossCheckLabels / CheckSequenceRows, #221). + return fmt.Errorf("reading %s: %w", filepath.Base(csvPath), rerr) + } + row++ + for _, c := range checks { + if c.idx >= len(rec) { + continue // short/ragged row — the read path owns that diagnostic + } + if !numericCellBad(rec[c.idx], c.integer) { + continue + } + o := offenses[c.name] + if o == nil { + o = &offense{integer: c.integer} + offenses[c.name] = o + } + o.count++ + if len(o.rows) < maxSample { + o.rows = append(o.rows, fmt.Sprintf("data row %d", row)) + } + } + } + if len(offenses) == 0 { + return nil + } + // Report the first offending column in header order, for a stable message. + for _, c := range checks { + o := offenses[c.name] + if o == nil { + continue + } + kind := "numbers" + if o.integer { + kind = "whole numbers (integers)" + } + return fmt.Errorf( + "column %q isn't all %s: %d value(s) don't match its declared type (e.g. %s). "+ + "The cluster's data-type check rejects these after the table is created — fix the "+ + "values, or correct the column's type with --schema, then re-run.", + c.name, kind, o.count, strings.Join(o.rows, ", ")) + } + return nil +} + +// numericCellBad reports whether a RAW cell violates its column's numeric +// contract, in the SAFE (never-over-reject) direction. See CheckColumnValueTypes +// for the full read semantics; in brief: NA-sentinel / whitespace-only cells are +// missing (not flagged); a present cell is bad iff it doesn't parse as a number +// (INT and FLOAT) or, for INT, parses to a finite value with a fractional part. +// inf/nan parse but are left to the ingestor's own non-finite handling +// (under-reject, safe). +func numericCellBad(raw string, integer bool) bool { + if _, isNA := naSentinels[raw]; isNA { + return false + } + t := strings.TrimSpace(raw) + if t == "" { + return false + } + f, err := strconv.ParseFloat(t, 64) + if err != nil { + return true // non-numeric — pandas' to_numeric coerces this to NaN too + } + if integer { + if math.IsInf(f, 0) || math.IsNaN(f) { + return false // left to the ingestor's non-finite / NA handling + } + if f != math.Trunc(f) { + return true // a fractional value in an INT column + } + } + return false +} diff --git a/internal/push/value_types_test.go b/internal/push/value_types_test.go new file mode 100644 index 00000000..77de0b4d --- /dev/null +++ b/internal/push/value_types_test.go @@ -0,0 +1,207 @@ +package push + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestCheckColumnValueTypes covers the DataValidator numeric per-value preview +// (cli#352). The through-line is the parity contract's cardinal rule: reject a +// value the cluster would reject (non-numeric / fractional in a numeric column), +// but NEVER over-reject — valid numbers, NA/missing cells, numeric-looking +// strings, and the deliberately-unmirrored types (string/date/boolean) all pass. +func TestCheckColumnValueTypes(t *testing.T) { + cases := []struct { + name string + csv string + schema map[string]string + category string + wantErr bool + wantSub string + }{ + // --- rejects: values that don't match a declared numeric type --- + { + name: "int non-numeric rejected", + csv: "age,label\nabc,x\n40,y\n", + schema: map[string]string{"age": "INT", "label": "VARCHAR(255)"}, + category: "tabular_classification", + wantErr: true, wantSub: `"age"`, + }, + { + name: "int fractional rejected", + csv: "age\n30\n40.5\n", + schema: map[string]string{"age": "INT"}, + category: "tabular_classification", + wantErr: true, wantSub: "whole numbers", + }, + { + name: "bigint non-numeric rejected", + csv: "n\n5\nNaNaN\n", + schema: map[string]string{"n": "BIGINT"}, + category: "tabular_classification", + wantErr: true, wantSub: `"n"`, + }, + { + name: "float non-numeric rejected", + csv: "price\n9.99\nxyz\n", + schema: map[string]string{"price": "FLOAT"}, + category: "tabular_regression", + wantErr: true, wantSub: `"price"`, + }, + + // --- accepts: never over-reject --- + { + name: "int 1.0 accepted (Excel-style whole float)", + csv: "age\n1\n1.0\n2\n", + schema: map[string]string{"age": "INT"}, + category: "tabular_classification", + wantErr: false, + }, + { + name: "int NA and empty accepted (missing, stored NULL)", + csv: "age,x\n30,a\n,b\nNA,c\nnull,d\n", + schema: map[string]string{"age": "INT", "x": "VARCHAR(255)"}, + category: "tabular_classification", + wantErr: false, + }, + { + name: "float scientific + padded + signed accepted", + csv: "price\n1e3\n 2.5 \n-0.1\n", + schema: map[string]string{"price": "FLOAT"}, + category: "tabular_regression", + wantErr: false, + }, + { + name: "float allows fractional (not integer-checked)", + csv: "price\n1.5\n2.75\n", + schema: map[string]string{"price": "DOUBLE"}, + category: "tabular_regression", + wantErr: false, + }, + { + name: "numeric-looking value in numeric column accepted (#188 direction)", + csv: "zip\n01000\n90210\n", + schema: map[string]string{"zip": "INT"}, + category: "tabular_classification", + wantErr: false, + }, + + // --- deliberately unmirrored types (documented under-preview): accept --- + { + name: "varchar not per-value checked", + csv: "code\nabc\n123xyz!!\n", + schema: map[string]string{"code": "VARCHAR(3)"}, + category: "tabular_classification", + wantErr: false, + }, + { + name: "date not per-value checked", + csv: "d\nnot-a-date\n2024-13-99\n", + schema: map[string]string{"d": "DATE"}, + category: "tabular_classification", + wantErr: false, + }, + { + name: "boolean not per-value checked", + csv: "b\nmaybe\nsometimes\n", + schema: map[string]string{"b": "BOOLEAN"}, + category: "tabular_classification", + wantErr: false, + }, + + // --- scope / resolution edges --- + { + name: "empty schema is a no-op", + csv: "age\nabc\n", + schema: map[string]string{}, + category: "tabular_classification", + wantErr: false, + }, + { + name: "schema column absent from CSV is not this check's diagnostic", + csv: "other\n1\n", + schema: map[string]string{"age": "INT"}, + category: "tabular_classification", + wantErr: false, + }, + { + name: "case-sensitive column match (Age != age)", + csv: "Age\nabc\n", + schema: map[string]string{"age": "INT"}, + category: "tabular_classification", + wantErr: false, // schema 'age' doesn't match header 'Age' — CheckSchemaColumns owns that + }, + { + name: "whitespace-stripped header match", + csv: " age \nabc\n", + schema: map[string]string{"age": "INT"}, + category: "tabular_classification", + wantErr: true, wantSub: `"age"`, + }, + { + name: "TSC grouping time column excluded (fractional step accepted)", + csv: "sequence_id,timestamp,hr\np1,1.5,80\np1,2.5,84\n", + schema: map[string]string{"sequence_id": "VARCHAR(64)", "timestamp": "INT", "hr": "FLOAT"}, + category: "time_series_classification", + wantErr: false, + }, + { + name: "TSC feature column still checked", + csv: "sequence_id,timestamp,hr\np1,1,notnum\np1,2,84\n", + schema: map[string]string{"sequence_id": "VARCHAR(64)", "timestamp": "INT", "hr": "FLOAT"}, + category: "time_series_classification", + wantErr: true, wantSub: `"hr"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := filepath.Join(t.TempDir(), "data.csv") + if err := os.WriteFile(p, []byte(tc.csv), 0o644); err != nil { + t.Fatal(err) + } + err := CheckColumnValueTypes(p, tc.schema, tc.category) + switch { + case tc.wantErr && err == nil: + t.Fatalf("expected a rejection, got nil") + case !tc.wantErr && err != nil: + t.Fatalf("expected accept, got: %v", err) + case tc.wantErr && tc.wantSub != "" && !strings.Contains(err.Error(), tc.wantSub): + t.Errorf("error missing %q: %v", tc.wantSub, err) + } + }) + } +} + +// TestNumericCellBad pins the cell-level contract directly, including the safe +// (under-reject) handling of inf/nan and NA sentinels. +func TestNumericCellBad(t *testing.T) { + tests := []struct { + raw string + integer bool + bad bool + }{ + {"42", true, false}, + {"42", false, false}, + {"1.0", true, false}, // whole float in INT — Excel writes these + {"1.5", true, true}, // fractional in INT + {"1.5", false, false}, // fractional in FLOAT is fine + {"abc", true, true}, // non-numeric + {"abc", false, true}, // non-numeric + {"", true, false}, // empty = missing + {"NA", true, false}, // NA sentinel = missing + {"null", false, false}, // NA sentinel = missing + {" ", true, false}, // whitespace-only = missing (under-reject, safe) + {" 7 ", true, false}, // padded number parses after trim + {"1e3", false, false}, // scientific notation + {"inf", true, false}, // left to the ingestor's non-finite handling + {"inf", false, false}, + {"-5", true, false}, + } + for _, tc := range tests { + if got := numericCellBad(tc.raw, tc.integer); got != tc.bad { + t.Errorf("numericCellBad(%q, integer=%v) = %v, want %v", tc.raw, tc.integer, got, tc.bad) + } + } +}