Skip to content

feat(data ingest): confirm the inferred tabular schema (#185)#210

Merged
saadqbal merged 2 commits into
developfrom
feat/185-tabular-schema-confirm
Jul 10, 2026
Merged

feat(data ingest): confirm the inferred tabular schema (#185)#210
saadqbal merged 2 commits into
developfrom
feat/185-tabular-schema-confirm

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Under Option A (RFC-0002 §8.1), data-ingestors owns the tabular schema-inference rules (di#349) and the CLI mirrors them so the schema it EMITS is the same answer the ingestor would compute. This PR lands the CLI-side mirror: InferSchema infers a column → SQL type schema from the CSV, the CLI emits it explicitly as spec.schema, and a venv-free parity harness pins the two implementations to the same per-column answer.

Part of #185.

What changed

  • internal/push/tabular.goInferSchema / inferColumnType mirror di#349's rule precedence (first match wins): leading-zero code → BOOLEAN → INT/BIGINT → FLOAT → DATE/DATETIME → VARCHAR(n). VARCHAR(n) is sized by rune count (MySQL character semantics), integer/float matching is ASCII-only, and framework-managed columns are skipped. Empty / id-like columns are returned so the caller can surface them as warnings.
  • internal/cli/data.go — when no --schema is given, infer here and emit a.Spec.Schema → spec.schema explicitly; warn on skipped / empty-in-sample / id-like columns.
  • internal/push/schema_inference_parity_test.go + testdata/schema_inference_parity.json — the value-level parity contract, vendored byte-for-byte from di#349's fixture.

How it mirrors di#349 + the venv-free parity test

The ingestor's schema_inference.infer_column_type is the source of truth (Principle 6 / backend#1009). testdata/schema_inference_parity.json is vendored verbatim from di#349's tests/fixtures/schema_inference_parity.json — edit it once and both sides are pinned to the same answer. TestSchemaInferenceParity runs it as a static, venv-free check on every PR (no Python / pandas needed in CI), so drift between the Go mirror and the Python owner fails the build.

Explicit-emit rationale

The CLI does not rely on the deployed ingestor to re-infer. It emits spec.schema authoritatively, so the type each column lands as in-cluster is the answer the user saw and can override with --schema — independent of which ingestor version happens to be running. A mis-typed column is always correctable with --schema (the SAFE direction).

Schema ref deliberately NOT bumped

This PR does not touch the vendored ingest.v1.json schema ref. Emitting spec.schema uses the existing contract; nothing here changes the wire schema, so the pinned ref stays as-is (the drift tripwire remains green).

Review-finding fixes folded in

  • Mixed-timezone DATETIME parity divergence (fixed). A column of RFC3339 values with non-uniform UTC offsets (or a mix of tz-aware and tz-naive) was typed DATETIME by the CLI but VARCHAR by the ingestor — the one divergence in the unsafe direction (per-row offset silently dropped in-cluster). inferDatetime now detects a non-uniform timezone and routes to VARCHAR, mirroring _infer_datetime's None result on mixed timezones. Pinned by TestInferColumnType_TimezoneParity (verified against di#349 / pandas 3.0.3). The ASCII-only shared fixture is left untouched — di#349 carries no tz case, so adding one would diverge the vendored contract.
  • Dead code removed. SerializeSchema, the InferredColumn type, and SchemaInference.Columns were built for an interactive confirm/amend step that is not part of this PR and had zero callers/readers; removed, and the comments that described that step as if it existed are corrected to match the shipped behavior.
  • Stale --schema help refreshed to the full inferred type set.
  • VARCHAR-width and textual-BOOLEAN behavior kept as-is — both are faithful mirrors of di#349's owned rules (Principle 6); diverging the CLI from the source of truth would be its own parity bug.

Test plan

go build ./... && go test ./... && gofmt -l . (empty) && go vet ./... — all green locally. Parity + timezone subtests pass; existing tabular / interactive tests unchanged.


Hand-finished after the autonomous build stalled on the venv-goldens step; the type-inference mirror + parity harness were completed and the review findings addressed by hand.


Note

Medium Risk
Changes how tabular column types are chosen and sent on the wire; mistakes could mis-type data at load time, though parity tests and explicit --schema mitigate this, and VARCHAR fallbacks favor the safe direction.

Overview
Replaces the CLI’s simple INT/FLOAT/VARCHAR inference with a di#349-aligned mirror of data-ingestors’ schema_inference rules, so tabular ingests without --schema get the same column types the ingestor would compute.

InferSchema now returns a SchemaInference struct (schema map plus Skipped / Empty / IDLike warnings). Classification follows first-match-wins precedence: leading-zero codes → textual BOOLEAN → INT/BIGINT → FLOAT → DATE/DATETIME → VARCHAR(n) sized by rune count, with a mixed-timezone guard so non-uniform RFC3339 offsets fall back to VARCHAR instead of tz-naive DATETIME.

data ingest infers when --schema is omitted, emits spec.schema explicitly (authoritative in-cluster), and prints warnings for risky columns. TestSchemaInferenceParity plus vendored schema_inference_parity.json pin per-column types against the ingestor contract without Python in CI.

Reviewed by Cursor Bugbot for commit 4b88b49. Bugbot is set up for automated code reviews on this repo. Configure here.

@LukasWodka LukasWodka closed this Jul 10, 2026
@LukasWodka LukasWodka force-pushed the feat/185-tabular-schema-confirm branch from ec76b65 to d57a6d5 Compare July 10, 2026 07:22
@LukasWodka LukasWodka reopened this Jul 10, 2026
LukasWodka and others added 2 commits July 10, 2026 09:46
Mirror data-ingestors' di#349 tabular type-inference in the Go CLI
(InferSchema → *SchemaInference), show the inferred col:TYPE for the 4
tabular/time tasks, let the user confirm/amend, and EMIT the confirmed
schema explicitly as spec.schema — so the ingestor uses the CLI's
(di#349-correct) types regardless of its deployed version. Leading-zero
codes stay VARCHAR (the #349 fix), id-like INT and empty-in-sample
columns are surfaced in the confirm step.

Pinned to di#349's value->type contract via a venv-free Go parity test
(testdata/schema_inference_parity.json, 21 cases). Inference is a pure-Go
mirror — no schema-ref bump needed (the min_size/efaeb07 drag-in belongs
to #183), so no validator-goldens regen.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Mixed-timezone DATETIME parity fix: a tabular column of RFC3339 values
  with non-uniform UTC offsets (or a mix of tz-aware and tz-naive) is now
  routed to VARCHAR, matching the ingestor's schema_inference._infer_datetime
  (pd.to_datetime(format="mixed") returns None on mixed timezones, di#349).
  Previously each offset token parsed individually via time.RFC3339 and the
  column was typed DATETIME, so the emitted spec.schema was a tz-naive
  DATETIME that silently dropped the per-row offset in-cluster. Pinned with
  TestInferColumnType_TimezoneParity (verified against di#349 / pandas 3.0.3);
  the shared ASCII-only parity fixture is left untouched — di#349 carries no
  tz case, so adding one would diverge the vendored contract.

- Remove dead API surface built for an interactive confirm/amend step that
  is not part of this PR: SerializeSchema, the InferredColumn type, and
  SchemaInference.Columns had zero callers/readers repo-wide.

- Correct comments that described that unbuilt step as if it existed
  (data.go schema branch; the InferSchema / SchemaInference docs): the CLI
  infers the schema mirroring di#349 and EMITS it explicitly; the interactive
  prompt captures an optional --schema override; risky columns are surfaced
  as warnings.

- Refresh the stale --schema flag help to the full inferred type set
  (INT/BIGINT/FLOAT/BOOLEAN/DATE/DATETIME/VARCHAR(n)).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbal

Copy link
Copy Markdown
Collaborator

Nice, careful PR — parity discipline is solid and I couldn't find a correctness bug (verified the datetime/type/all-null cases against di#349 and the current ingestor). Two small things, neither blocking:

  • varcharOf sizes VARCHAR(n) from the 5000-row sample, so we've lost the old fixed VARCHAR(255) floor. A string longer than the sampled max that only shows up past the cap gets an undersized column and truncates/errors in-cluster after the upload. This faithfully mirrors di#349 so I'm not asking to change the rule — just worth a line in the inferred-schema warning (or docs) that the width is sample-derived and --schema is the escape hatch.
  • cleanTokens re-caps at schemaInferenceSampleRows, but InferSchema's read loop already bounds cols[i] to 5000, so that cap only ever fires for the test-only inferColumnType. Either drop it or move it to where it's actually load-bearing, so nobody reads it as guarding the real path.

@saadqbal saadqbal merged commit 88f2bf3 into develop Jul 10, 2026
17 checks passed
@saadqbal saadqbal deleted the feat/185-tabular-schema-confirm branch July 10, 2026 10:36
LukasWodka added a commit that referenced this pull request Jul 10, 2026
#208's fix #2 (demote Inf/NaN columns to VARCHAR) is superseded by #185/#210:
the di#349 floatRE grammar pre-screens the token before ParseFloat, so
"inf"/"Infinity"/"NaN" already fall through to VARCHAR. Dropped the redundant
production change on rebase; kept the intent as a regression test, since no
parity-fixture case covers non-finite.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka added a commit that referenced this pull request Jul 10, 2026
…canner buffer (bug-hunt MED) (#208)

* fix(auth): reject unknown --env/$CLIENT_ENV at login instead of silently using prod

BaseURL falls unknown/typo env values back to prod (a lenient library
default), so `login --env staging` or `CLIENT_ENV=prd` silently targeted
production AND persisted it as the active session env for every later
command — the class behind earlier dev-vs-prod confusion.

login PICKS and persists the session env, so a typo must fail there. Add
api.IsKnownEnv (dev/stg/prod, case-insensitive) and validate the resolved
env at runLogin entry, before any network call. ResolveEnv still maps
empty->prod, so the no-flag default is unaffected. BaseURL's unknown->prod
fallback is deliberately unchanged (TestBaseURL asserts it).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(submit): raise log-scanner buffer to 16 MB so tqdm progress can't force a false exit 9

tqdm (a data-ingestors dep) redraws its progress bar with \r and no \n,
so a whole ingestion phase's redraws are one newline-delimited "line" that
grows for the life of the run. Past 1 MB the display scanner returned
bufio.ErrTooLong, cutting the log stream mid-run; a still-running Job then
couldn't be confirmed terminal in the 30s finalJobStatus poll, so watch
returned a false exit 9 on a healthy large ingestion — exactly the case the
1h JobWatchTimeout targets.

The parser is fed via the TeeReader, not the scanner, so this cap only ever
bounded the DISPLAY line and never the verdict. Raise it to 16 MB (clears a
fast ~10/s hour of redraws with headroom; the 1h cap bounds accumulation).
The buffer grows on demand, so ordinary log lines still cost 64 KB.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(submit): drain past ErrTooLong so a giant tqdm line can't force a false exit 9 (review)

Addresses @saadqbal's review on #208: the 16 MB buffer bump only MOVED the
false-exit-9 threshold, it didn't close it. The tee is pulled only by the
DISPLAY scanner, so when a line trips ErrTooLong the scan loop exits, the tee
stops being read, and the parser never sees the rest of the stream (the closing
banner) → streamFailed && outcome==Unknown → a false exit 9 on a healthy run.
A long enough single '\r'-line (> the buffer) still breaks it.

Class-level fix (his suggestion): keep draining past ErrTooLong. Extracted the
display/parse loop into streamDisplayAndParse; on ErrTooLong it drains the rest
of the stream THROUGH the tee (io.Copy to io.Discard) so the parser still sees
the banner, and it is NOT fatal — the Job status poll is the verdict's source of
truth. Genuine read failures (network drop, ctx cancel) still propagate.
Corrected the now-wrong "cap never affects the verdict" comment; kept 16 MB as a
generous display headroom (the drain is the correctness guarantee).

New tests (the #3 no-test gap Asad noted): an oversized '\r'-line + the real
ingestor banner → the parser still resolves the summary (no false exit 9); and
a genuine read error still propagates.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test(push): pin Inf/NaN → not FLOAT against the di#349 inference

#208's fix #2 (demote Inf/NaN columns to VARCHAR) is superseded by #185/#210:
the di#349 floatRE grammar pre-screens the token before ParseFloat, so
"inf"/"Infinity"/"NaN" already fall through to VARCHAR. Dropped the redundant
production change on rebase; kept the intent as a regression test, since no
parity-fixture case covers non-finite.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants