Repository navigation
python(feat): support bit field definitions on import configs - #861
alden-sift wants to merge 13 commits into
Conversation
|
Python docs preview: https://sift-stack.github.io/sift/python/pr-861/ Deployed from |
This reverts commit bc54be6.
| If neither ``run`` nor ``run_name`` is provided (and none is | ||
| set on the config), ``run_name`` defaults to the filename. | ||
|
|
||
| ``BIT_FIELD`` values must be raw bytes, a string of 0s and 1s, or |
There was a problem hiding this comment.
is this the right place for this?
There was a problem hiding this comment.
I believe so, but it can be better prefaced with something like "For files containing bit field datatype columns, …". Otherwise, users using the import_from_path method for such files won't discover this rule without digging into bit field parsing code.
alexluck-sift
left a comment
There was a problem hiding this comment.
Not blocking: do you expect users to need units, a description or signedness per element? Elements only carry a name, index and width, so that would be a proto change to ChannelBitFieldElement. Worth tracking separately if so?
| bit_field_elements: Named bit ranges within each value. Only valid when | ||
| ``data_type`` is ``ChannelDataType.BIT_FIELD``, and only on formats | ||
| that support bit fields. |
There was a problem hiding this comment.
Elements only carry a name, index and width, so units and description here describe the whole status word, and from your tables each element comes back as an unsigned int. Does the import service put the parent's units on status.armed and the other sub-channels? Either way, I'd say both things in this docstring and the changelog, since a user setting units on a bit field column will expect it per element.
There was a problem hiding this comment.
Tested on dev - the parent's units and descriptions do show up for each sub-channel. Docstring and changelog is updated accordingly in cb935df
There was a problem hiding this comment.
I can see a need for each element to have its own unit and description. Can't make an informed comment on signedness, but quick google says that it is implementable. I can make the follow up issue in linear.
| units=d.channel_config.units, | ||
| description=d.channel_config.description, | ||
| enum_types=_enum_types_from_proto(d.channel_config), | ||
| bit_field_elements=_bit_field_elements_from_proto(d.channel_config), |
There was a problem hiding this comment.
UlogDataColumn rejects bit fields, so this read can only return None or make _from_proto raise on a proto that carries elements. Same at the MCAP read below. I'd drop it in both places and keep the enum_types read.
| bit_field_elements=_bit_field_elements_from_proto(d.channel_config), |
| units=d.channel_config.units, | ||
| description=d.channel_config.description, | ||
| enum_types=_enum_types_from_proto(d.channel_config), | ||
| bit_field_elements=_bit_field_elements_from_proto(d.channel_config), |
There was a problem hiding this comment.
Same here, since McapDataColumn rejects bit fields too.
| bit_field_elements=_bit_field_elements_from_proto(d.channel_config), |
| _check_values(dc, samples[i]) | ||
|
|
||
|
|
||
| def _check_parquet(path: Path, columns: Mapping[str, DataColumnBase]) -> None: |
There was a problem hiding this comment.
These Parquet helpers re-import pyarrow per function and copy the missing-pyarrow message from detect_config. _internal/util/parquet.py already imports pyarrow at the top and owns PATH_SEPARATOR, so I'd move _check_parquet, _field_type and _column_values there and keep one guarded import here, the same way detect_config imports detect_flat_parquet_config.
Co-authored-by: Alex Luck <luck@siftstack.com>
What was changed
CSV, Parquet, TDMS and HDF5 import configs can now define bit field channels. Before this,
DataColumnBasehad nobit_field_elementsfield, so a config fromdetect_configcouldn't describe aBIT_FIELDchannel.What's a bit field
A bit field is one integer that packs several small values into fixed bit ranges. Hardware status words are the common case. In this 8-bit status value, bit 0 says whether the vehicle is armed, bits 1 to 3 hold an error code, and bits 4 to 7 hold a mode:
The raw value
53(0b0011_0101) decodes toarmed=1,error_code=2,mode=3. Sift splits aBIT_FIELDchannel into one sub-channel per element (status.armed,status.error_code,status.mode), but it needs the layout to do that. Each element is aChannelBitFieldElementwith aname, a starting bitindexand abit_countwidth.detect_configcan't infer the layout, so the user supplies it:Value formats
In CSV and Parquet, a bit field value must be raw bytes, a string of 0s and 1s, or URL-safe base64. Bytes are little-endian. Integers aren't accepted, and Parquet has no bit field type, so the client honors the config and expects the data in one of those formats. It doesn't rewrite files.
TDMS and HDF5 store typed integers, and an integer channel imports as its little-endian bytes, so those files need no conversion.
import_from_pathchecks CSV and ParquetBIT_FIELDcolumns before uploading, so a wrong format fails before the file is sent:10(bit string) or1234(base64), since the client can't tell those from intended input.Other changes
DataColumnBasegetsbit_field_elements. A validator requires it whendata_typeisBIT_FIELDand rejects it otherwise. It also rejects duplicate names, negative indexes, zero-width elements and overlapping ranges.BIT_FIELD, since those imports don't support bit field overrides. The field is onDataColumnBase, soUlogDataColumnandMcapDataColumnopt out through a_supports_bit_fieldsclass flag, and the validator raises when they're set toBIT_FIELD._channel_config_proto()writes the elements, and every_from_protoreads them back, including the embedded Parquet config path._channel_config_proto()re-runs the enum and bit field checks. Pydantic doesn't validate on assignment, so an in-place edit like the one above could otherwise send aBIT_FIELDchannel with no elements.enum_types. MCAP built its channel config by hand. It now uses the shared helper and reads enums back.Multi-channel single-channel-per-row Parquet still doesn't support bit fields, same as enums, because that layout derives channel configs per row.
Testing
BIT_FIELDat construction and on in-place edits, each validation error, in-place edits caught at import, and the pre-upload checks: accepted and rejected Parquet types, nested fields, sampled CSV and Parquet string values, unpadded bit strings and digit-only base64 passing, and a failing check stopping the import before anything is created or uploaded.binary,fixed_size_binary, bit string and base64 string columns, a 16-bit Parquet status word, and CSV bit strings and base64, and TDMS and HDF5uint8,uint16andint16channels all import and decode to the expected element values. The 16-bit cases decode0x1234tolow=0x34,high=0x12, so multi-byte values are little-endian.Verification on a local stack
Each case wrote a synthetic file, imported it into a local stack and read the elements back:
Accepted formats. All six imports succeeded and created a
BIT_FIELDchannel with the three elements.binaryfixed_size_binary[1]string, bitsstring, base640x350x3500110101NQ==00110101NQ==0x000x0000000000AA==00000000AA==0xff0xff11111111_w==11111111_w==0x010x0100000001AQ==00000001AQ==All six returned identical values, matching the hand-decoded expectation.
16-bit status word.
0x123434 120x00ffff 000xff0000 ff0x000101 00TDMS and HDF5 integer channels. Each file held one integer
statuschannel, set toBIT_FIELDafterdetect_config:TDMS and HDF5 returned identical values:
uint8uint160x1234,0x00ff,0xff00,0x0001int160x1234, -1, -256, 1Rejected before upload. Each raised a
ValueErrorbefore an import was created. A bit field value is never read as a decimal integer, so a CSV cell53isn't a bit string or valid base64. It has to be written as00110101,NQ==or the byte0x35.uint8list<uint8>Parquet
uint8:Parquet
list<uint8>:CSV: