Repository navigation
feat(query): add atb columns and validate --columns names - #42
Merged
Merged
Conversation
Plan() reads sylph.parquet and mlst.parquet when one of their columns is requested, but buildRow() never carried the values onto the result, so Adjusted_ANI, Taxonomic_abundance, Sequence_abundance, Median_cov and the five mlst_* columns came back as empty strings whenever a query could not be answered from the SQLite index. The map lookup in Execute turns a missing key into "", so the query exited 0 and looked like a sample with no MLST record rather than a gap in the executor. Carry both tables through sampleData the way checkm2 and assembly_stats already are, and add a sylph fixture so the executor tests cover the join rather than the file being opened.
The list named organism, tissue, age, sex and disease, none of which are columns in this dataset, and nothing referenced it. Default output comes from InferColumns, which reads the keys the query actually produced.
There was no way to find out which names --columns accepts, and a name that was not a column produced a blank column and exit status 0, so a typo looked like missing data. Add internal/columns, one registry of the 40 columns with the table each comes from and whether the SQLite index can answer it. atb columns prints it grouped by table, or as tsv/csv/json for scripts, and reads no data so it works before anything has been downloaded. docs/reference/columns.md is generated from the same registry by make docs, and the CI staleness gate now covers it. atb query rejects an unrecognised name and exits 1 before opening the index, naming the closest columns when any are close. Names from a TOML filter file are checked after the merge, since they never pass through the flag. This is a breaking change for scripts that pass a name that is not a column. Suggestions use a new suggest.Identifiers rather than suggest.Suggest, whose dist*4 <= maxLen gate is tuned for species names and demands an exact match at three characters, so "N5O" would suggest nothing. Identifiers accepts a distance of 2, the bound cobra uses for command names. Two tests keep the registry honest: one checks that Plan reads the source file of every registered column and that every column Plan routes is registered, the other that InIndex agrees with userColToSQL. Closes #34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34.
--columnshad no discoverable list of names, and a name that was not a column silently produced a blank column and exit status 0. Auditing the accepted names to answer the issue turned up a second problem: nine of them never returned data on the parquet path.atb columnsA new command lists all 40 columns, grouped by the table each comes from, with
*marking the ones the SQLite index cannot answer:--format tsv|csv|jsonprints the same list for scripts. The command reads no data, so it works before anything has been downloaded.Validation (breaking)
An unrecognised name is now an error, raised before the index is opened:
Names from a TOML filter file are checked too, after the merge, since they never pass through the flag. Scripts that pass a name that is not a column will start failing where they previously exited 0 with a blank column.
Suggestions use a new
suggest.Identifiers.suggest.Suggest'sdist*4 <= maxLengate is tuned for species names and demands an exact match at three characters, soN5Owould have suggested nothing.Identifiersaccepts a distance of 2, the bound cobra uses for command names.Fix: sylph and mlst returned nothing on the parquet path
Plan()readssylph.parquetandmlst.parquetwhen one of their columns is requested, butbuildRow()never carried the values onto the result. The map lookup inExecuteturns a missing key into"", so the query exited 0 and looked like a sample with no MLST record rather than a gap in the executor.Before / after, on real data:
The two execution paths now agree. Asking the SQLite fast path and the parquet path for the same two samples returns identical rows, including a genuinely absent MLST record:
Guarding against the next drift
The defect class here is two lists of the same thing that nothing compared:
Plan()decided which files to read from the column list,buildRow()decided which keys to emit from a hardcoded struct. Three tests now hold the registry to the code:Planreads the source file of every registered column.Planroutes is registered, under the same source.InIndexagrees withuserColToSQL, in both directions.Each was verified by breaking the invariant deliberately and checking the failure message before being kept.
Docs
docs/reference/columns.mdis generated from the registry bymake docs, moving it out of the hand-written tier that drifts silently. The CI staleness gate now covers it as well asdocs/reference/cli. The query guide links to it.output.DefaultColumnsis deleted in its own commit: it listedorganism,tissue,age,sexanddisease, none of which are columns in this dataset, and nothing referenced it.