Skip to content

feat(push): infer entirely-empty columns as nullable FLOAT + warn - #66

Merged
saadqbal merged 1 commit into
developfrom
harden/infer-empty-columns-float
Jun 8, 2026
Merged

feat(push): infer entirely-empty columns as nullable FLOAT + warn#66
saadqbal merged 1 commit into
developfrom
harden/infer-empty-columns-float

Conversation

@LukasWodka

Copy link
Copy Markdown
Contributor

Summary

InferSchema typed an entirely-empty column (no non-empty value in the sample) as VARCHAR(255). That column then fails ingest: data-ingestors' string validator rejects the all-NULL column. Sparse panels hit this on every empty column.

This types such columns as a nullable FLOAT instead (the FLOAT validator accepts NULLs, so an all-empty column ingests cleanly as NULLs), and surfaces them so dataset push warns the user.

Why

A real biomarker panel (proteomics matrix) — an analyte measured in zero samples is an all-empty column:

patient_id,Sex,...,P02751-1|FN1,P05109|S100A8
PAT000001,female,...,,18.6198268 # FN1 empty for every patient

FN1 (empty everywhere) inferred VARCHAR(255) → ingest failed with Column 'P08254|MMP3' contains 207 non-string values (207 patients, none measured). Sparse panels always have unmeasured analytes, so this bit every empty column.

What

  • InferSchema: a column with no non-empty sampled value → FLOAT (was VARCHAR(255)), and is returned in a new empty []string so the caller can warn. The numeric path is unchanged (all-int → INT, all-numeric → FLOAT, otherwise VARCHAR).
  • dataset push prints a warning naming the empty columns (typed FLOAT/nullable; override with --schema if a column is really text).
  • Updated the EmptyColumn test (now expects FLOAT + checks the empty return) and the two other InferSchema callers for the new signature.

gofmt + go vet + go test ./internal/push/ ./internal/cli/ all pass.

Pairs with

data-ingestors#167 — fixes the validator so a NULL is never miscounted as "non-string." This CLI change is the primary unblock (empty col → FLOAT → passes); #167 is the safety net for genuinely-text columns that have blanks. Either alone helps; together they close the sparse-tabular trap.

Note / open question for review

I changed the documented empty→VARCHAR(255) fallback to empty→FLOAT. Rationale: a tabular feature column is numeric far more often than text, and FLOAT-nullable ingests where all-NULL VARCHAR doesn't (and it also covers the "empty in the first 5k sampled rows, numeric later" edge). The warning + --schema override cover the rare genuinely-text-empty column. Happy to flip to "warn-only, keep VARCHAR, rely on #167" if you'd rather not change inferred types — your call.

🤖 Generated with Claude Code

When InferSchema saw a column with no non-empty value in the sample it fell
back to VARCHAR(255). An entirely-empty column then fails ingest: the
data-ingestors string validator rejects the all-NULL column ("N non-string
values"). Sparse panels (e.g. a proteomics matrix where an analyte is
measured in zero samples) hit this on every empty column.
Type such columns as a nullable FLOAT instead — the FLOAT validator accepts
NULLs, so an all-empty column ingests cleanly — and return them in a new
`empty` slice so `dataset push` warns which columns were empty (the user can
--schema-override if a column is really text).
Updates the renamed test + the InferSchema callers for the new return.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka
LukasWodka requested a review from saadqbalJune 8, 2026 08:07
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

👋 Heads-up — Code review queue is at 18 / 8

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

@saadqbal
saadqbal merged commit 5ca8a7b into developJun 8, 2026
15 checks passed
saadqbal added a commit that referenced this pull request Jun 8, 2026
…#64)
* fix(install.sh): persist PATH to the shell rc (parity with install.ps1) (#61)
* fix(install.sh): persist PATH to the shell rc (parity with install.ps1)
When the binary lands in the ~/.local/bin fallback (the normal unprivileged
`curl | sh` case), the installer now appends the PATH entry to the rc file the
user's shell actually reads — instead of only printing advice.
Fixes the recurring "CLI still not in PATH after restarting the shell" ticket.
install.ps1 already persists user PATH on Windows; this brings Unix to parity.
The print-only behavior silently failed on Ubuntu: ~/.profile adds ~/.local/bin
only at login and only if it already existed, but the installer creates it
mid-session, so a new (non-login) terminal reading ~/.bashrc never saw it.
- Detects the rc per shell + OS: zsh -> .zshrc, bash -> .bashrc (Linux) /
.bash_profile (macOS login shell), fish -> config.fish, else .profile.
- Idempotent (skips if the rc already references the dir); falls back to
printing the line if the rc isn't writable.
Validated: sh -n + bash -n; functional + idempotency + per-shell routing tests.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: add install troubleshooting guide + harden rc-write fallback
Completes the install-PATH work tracked in #60. The installer change
already on this branch fixes the Ubuntu "not on PATH after restart"
report; this adds the troubleshooting doc + README link that #60 also
asked for, plus a small hardening of the rc-write fallback.
- docs/troubleshooting.md: command-not-found / PATH guide covering the
~/.local/bin fallback, login vs non-login shells (Linux + macOS), the
/usr/local/bin alternative, the Windows new-window case, and SSH login
shells.
- README: link the guide from the install section.
- install.sh: group the rc append as `{ …; } 2>/dev/null` so a read-only
or unwritable rc no longer leaks a raw "Permission denied" line before
the clean fallback message. Verified leak-free under dash + bash; all
shell/OS routing and idempotency unchanged.
Refs #60.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Asad Iqbal <asad.dsoft@gmail.com>
* feat(push): infer entirely-empty columns as nullable FLOAT + warn (#66)
When InferSchema saw a column with no non-empty value in the sample it fell
back to VARCHAR(255). An entirely-empty column then fails ingest: the
data-ingestors string validator rejects the all-NULL column ("N non-string
values"). Sparse panels (e.g. a proteomics matrix where an analyte is
measured in zero samples) hit this on every empty column.
Type such columns as a nullable FLOAT instead — the FLOAT validator accepts
NULLs, so an all-empty column ingests cleanly — and return them in a new
`empty` slice so `dataset push` warns which columns were empty (the user can
--schema-override if a column is really text).
Updates the renamed test + the InferSchema callers for the new return.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix(install.sh): tighten PATH idempotency + persist user-local PREFIX (Bugbot on #61) (#65)
* fix(install.sh): persist user-local PREFIX even when on PATH; tighten rc idempotency
Two Cursor Bugbot findings on #61's PATH-persistence block, both of which
let the installer print a false "Added ... to your PATH" while a new shell
still couldn't find the binary — the exact failure #61 set out to kill.
1. Loose idempotency: `grep -F "$PREFIX"` matched the directory anywhere in
the rc, including a bare comment, so a stray mention set added=1 with no
export written. Now strip comment lines, keep only real
PATH=/fish_add_path lines, then fixed-match the prefix.
2. Session PATH skipped the rc: the outer `case ":$PATH:"` skipped rc
persistence whenever $PREFIX was on PATH for the current shell, so a
one-off `export` (which won't survive a new terminal) left the rc
untouched. Branch on the install location instead — a user-local prefix
(under $HOME) always persists; a system prefix (e.g. /usr/local/bin) is
already on PATH for every shell and only gets a nudge if it isn't.
Validated: sh -n / bash -n / dash -n, shellcheck clean, and a functional
matrix over the extracted block (fresh, idempotent re-run, comment-only,
unrelated PYTHONPATH= line, system-prefix nudge, session-on-PATH persist,
plus zsh/fish/macOS-bash routing) — 9/9.
Refs #61
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(install.sh): tolerate a trailing slash in $HOME for the user-local match
Bugbot on #65: `case "$PREFIX" in "$HOME"/*)` misclassifies a user-local
prefix as a system one when $HOME ends in a slash (e.g. HOME=/home/u/ with
--prefix /home/u/.local/bin -> pattern /home/u//* never matches the single-
slash path), so rc persistence is skipped. Normalize with home_dir="${HOME%/}"
(POSIX suffix strip) before matching.
Functional matrix extended with a trailing-slash-HOME case — 10/10; sh/bash/
dash -n + shellcheck clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(install.sh): accurate present/added/failed messaging; recognize PATH+= and zsh path+=()
Two more Bugbot findings on #65:
- The user-local branch's failure message claimed "$PREFIX is not on $PATH",
which is wrong now that the branch runs regardless of session PATH (a one-off
export could already include it). Replaced the added/else pair with explicit
present/added/failed states so each message is accurate; the failed case no
longer asserts PATH state.
- The idempotency filter only matched PATH=/fish_add_path, missing the PATH+=
append idiom (and zsh's path+=() array), so a manual entry of that form wasn't
recognized and a re-run could append a duplicate block. Broadened to
case-insensitive (^|[^A-Za-z_])path[+]?= (still excludes PYTHONPATH=/MYPATH=).
Functional matrix now 13/13 (adds PATH+=, zsh path+=(), persist-failure message);
sh/bash/dash -n + shellcheck clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(install.sh): persist any off-PATH prefix to rc, not just $HOME-local ones
Bugbot on #65: keying rc persistence on `case "$PREFIX" in "$home_dir"/*)`
dropped writable non-$HOME targets (e.g. `--prefix /opt/tracebloc`) into the
system branch, which only nudges — so a custom off-PATH install no longer
persisted and a new terminal couldn't find tracebloc. cli#61's original code
persisted ANY off-PATH prefix.
Compute a `persist` decision instead: yes when the prefix is under $HOME
(always — survives a new terminal even if a session export already has it) or
when it's a non-$HOME prefix not on the current $PATH (e.g. /opt/...); no for a
non-$HOME prefix already on PATH (the default /usr/local/bin needs nothing). One
rc-writing block then handles every persist case.
Functional matrix 14/14 (adds off-PATH /opt prefix persists; default system
prefix on PATH stays silent); sh/bash/dash -n + shellcheck clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: lukasWuttke <54042461+LukasWodka@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka
LukasWodka deleted the harden/infer-empty-columns-float branch July 9, 2026 11:41
Sign up for freeto 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

@LukasWodka@saadqbal