Let an author keep a column to themselves - #993
Conversation
A CSV header beginning with an underscore is the crew's own record keeping -- who backfilled a trench, which drawing a run came from -- and no reader looks for meaning in it. This is the rule the inventory format already applies to files, applied to columns: strict about near-misses, indifferent to clean misses. The name can never collide with a field a model might later add, since pydantic makes a leading underscore a private attribute rather than a field. Annotations read it the same way, at the frame boundary rather than the file one, so a set holds what a stored set holds and never writes a column it could not read back.
Three findings from the CLI review of the private-column rule. A frame of rows and no columns built a set which wrote nothing: a saved set came back shorter than the one which wrote it. Refused now, where what went missing can still be named -- for a table whose every column is private, and for a frame which never had one. A dimension may not begin with an underscore, since a dimension is stated by a column and no set reads a private one. Stored sets drop private columns before any cell is read, so a declaration a private column cannot meet is no longer checked against it, and a table read from a file states the same set the frame does.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds underscore-prefixed private columns to annotation and inventory table handling. Shared filtering removes these columns before typing and validation. Rows without readable columns raise errors. Tests and inventory documentation cover the behavior. ChangesPrivate column handling
Merge Risk: ⚪ Minimal · up to The change allows leading-underscore author columns while preserving existing modeled data behavior and adds safeguards against row loss; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #993 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 202 202
Lines 27343 27364 +21
=========================================
+ Hits 27343 27364 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bba420a827
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| frame, _ = read_parquet(path, what=what, empty=True) | ||
| if not len(frame.columns): | ||
| return None | ||
| frame = _kept_columns(frame, path) |
There was a problem hiding this comment.
When an annotation Parquet file marks an underscore-prefixed column in dascore:documents, read_parquet() parses and validates that column's JSON before _kept_columns() removes it. Consequently, malformed or otherwise non-DASCore content in a private column still raises ParameterError, unlike the CSV path and contrary to the rule that private-column contents are never interpreted. The private columns need to be excluded before read_parquet() performs document-column decoding.
Useful? React with 👍 / 👎.
| @pytest.mark.skipif(pyarrow is None, reason="pyarrow is not installed") | ||
| class TestPrivateColumns: |
There was a problem hiding this comment.
When PyArrow is not installed, this insertion attaches the existing skipif marker to the new CSV-only TestPrivateColumns class and leaves the following TestParquet class unmarked. The supported no-PyArrow test environment will therefore skip the tests that need no optional dependency while running the Parquet tests, which call to_parquet() and _forge() and fail because PyArrow is absent; move or duplicate the marker so it still decorates TestParquet.
Useful? React with 👍 / 👎.
1 participant
Description
Operators want somewhere to write down how something was actually deployed — who backfilled a trench, which drawing a run came from — beside the rows it is about, in the CSV they already maintain. Today a column the format does not model is refused:
notesreaches the model as an unknown field and raises a pydantic error which namesattrs.yamlrather than the CSV.This adds one rule, in the shape the inventory format already uses for files: a column whose header begins with an underscore is the author's own, and no reader looks for meaning in it.
_crew,_drawing,_who_backfilled_it— the values stay in the file, and nothing types them, checks them against a declaration, or refuses them. It is the same bargain as a hidden file: the module docstring already says loading is "strict about near-misses and indifferent to clean misses", and a private header is a column declining to participate.The name can never collide with a field a model might later add, since pydantic makes a leading underscore a private attribute rather than a field.
Two formats read hand-authored tables, and both take the rule. The inventory drops private columns as each track table is read, before the geometry table's numeric rule or the model's unknown-field check can see them. Annotation sets drop them where a frame enters the set, so a set built in memory and a set read from a file hold the same columns and no set writes a column it could not read back; stored sets also drop them before any cell is typed, so a dtype declared for a private column is not checked against it.
A note which should travel with the data still belongs in a field the model has — every inventory object has
description, and adescriptioncolumn already worked before this PR.Rows a table cannot write
Dropping columns exposed an older silent loss, which the CLI review caught: a frame with rows and no columns built a set of that many annotations, and writing it produced a table with no rows, so a saved set came back shorter than the one which wrote it. That is now refused where what went missing can still be named — for a table whose every column is private, for a frame which never had one, and with the file named when it came from one.
Two smaller findings from the same review: a set may no longer declare a dimension beginning with an underscore (a dimension is stated by a column, and no set reads a private one), and
_read_cellskeeps the index of the table it read, so a row is not lost between the file and the set.Changelog
Checklist
I have:
docs/contributing/general_guidelines.qmd).I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
descriptionfor notes that should be retained.