Repository navigation
python(feat): add client side detect config for parquet - #819
alden-sift wants to merge 19 commits into
Conversation
|
Python docs preview: https://sift-stack.github.io/sift/python/pr-819/ Deployed from |
Detect PARQUET_FLATDATASET import configs locally from the Parquet footer schema with pyarrow instead of calling the server DetectConfig endpoint. Detection matches the server: struct fields are flattened, the first top-level time typed column is the time column, and configs embedded by Sift exports are used directly. The time column is excluded from the data columns, and an INT64/UINT64 column named ts, timestamp, or time is used when no time typed column exists. Add relative_start_time to detect_config and import_from_path. It applies to Parquet only and warns when ignored for other formats or absolute time formats. Add a parquet extra for pyarrow. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Python 3.8 resolves pyarrow 17, which cannot build float16 arrays from Python floats or write view types to Parquet. Build the float16 column from numpy values and move the view types into a test that is skipped when the installed pyarrow cannot write them.
| if proto.time_column.format == TimeFormatProto.TIME_FORMAT_UNSPECIFIED: | ||
| raise ValueError( | ||
| "timestamp format is not supported. Please specify the time format explicitly." | ||
| ) |
There was a problem hiding this comment.
Why check the proto format here at all? detect_config already falls back to the caller's time_format, then ABSOLUTE_UNIX_NANOSECONDS (lines 416-422), but this raises before either runs. Making CsvTimeColumn.format optional like ParquetTimeColumn.format would let that fallback handle it.
There was a problem hiding this comment.
CsvImportConfig._from_proto calls TimeFormat(proto.time_column.format) as the format of the CsvImportConfig. TimeFormat does not have a TIME_FORMAT_UNSPECIFIED member, so we run into unhandled errors. This all happens during the _detect_config_for_type call on line 415, before the fallback.
Fixed in commit 11c7d84, where TimeFormat gains the UNSPECIFIED member and is handled with the existing fallback.
There was a problem hiding this comment.
Let's not add UNSPECIFIED to the public enum, since users could then pass it
There was a problem hiding this comment.
removed UNSPECIFIED from public enum and instead checks for valid enum membership in CsvImportConfig._from_proto in commit 906b4ad
| if not isinstance( | ||
| config, (ParquetFlatDatasetImportConfig, ParquetSingleChannelPerRowImportConfig) | ||
| ): |
There was a problem hiding this comment.
For single-channel-per-row the server-detected time column is built in ParquetTimeColumn._from_proto, and if the server reports a relative format the validator raises there, before this function can apply the caller's start time. I'd pass relative_start_time into the server path's _from_proto too, or scope the docstring to flat datasets.
There was a problem hiding this comment.
scopped to flat dataset for now, implemented in commit 9a4b196
| from sift.common.type.v1.channel_config_pb2 import ChannelConfig as ChannelConfigProto | ||
|
|
||
| from sift_client._internal.util.file import extract_parquet_footer | ||
| from sift_client.resources.data_imports import _infer_time_column |
There was a problem hiding this comment.
nit: this is the only detector in _internal/util that reaches up into resources/, and for a private helper. I'd move _infer_time_column and its two constants into a pyarrow-free _internal/util module and import it from both places.
| ) | ||
| return | ||
| time_format = config.time_column.format | ||
| if time_format is None or not time_format.name.startswith("RELATIVE_"): |
There was a problem hiding this comment.
nit: this is the third name.startswith("RELATIVE_") check (also data_import.py:143 and :837). An is_relative property on TimeFormat would keep them from drifting.
| parquet = ["pyarrow>=17.0.0"] | ||
|
|
||
| [tool.sift.extras.combine] | ||
| # Combined extras configurations and aliases can be defined here |
There was a problem hiding this comment.
make sure to update this to reflect any new extras. parquet isn't in file-imports (line 281).
| relative_start_time: Start time for a relative time format. | ||
| Parquet only; ignored with a warning for other formats or |
There was a problem hiding this comment.
why is this parquet only? Pretty sure it is supported in other formats? CsvTimeColumn, TdmsImportConfig, and Hdf5ImportConfig all have relative_start_time.
| data_type: DataTypeKey | None = None, | ||
| time_format: TimeFormat | None = None, | ||
| *, | ||
| relative_start_time: datetime | None = None, |
There was a problem hiding this comment.
datetime does not have nanosecond precision. Does the backend not support ns precision? if not, we really need to support it to ensure no data loss here. The proto field is a google.protobuf.Timestamp, which carries nanoseconds.
There was a problem hiding this comment.
fixed in commit a6c19e3 . Opting for pandas Timestamp class as it is more ergonomic than the protobuf Timestamp class, and as it subclasses datetime, keeps the code backwards compatible/non breaking.
|
|
||
| data_type_key = _resolve_data_type_key(path.suffix.lower(), data_type) | ||
| config = await self._detect_config_for_type(path, data_type_key) | ||
| config = await self._detect_config_for_type(path, data_type_key, relative_start_time) |
There was a problem hiding this comment.
nit: recommend using key-value args in almost all situations to avoid issues with ordering
| config.time_format = time_format | ||
|
|
||
|
|
||
| def _apply_relative_start_time(config: ImportConfig, relative_start_time: datetime) -> None: |
There was a problem hiding this comment.
nit: recommend moving all of these private functions to _internal since they are not class members. This would be really messy for a user to read
There was a problem hiding this comment.
moved helpers into _internal, and moved some config-specific helpers into ImportConfig methods in efe86ef
Co-authored-by: Alex Luck <luck@siftstack.com>
Import config fields accept a pd.Timestamp alongside datetime, and configs read from protos return a pd.Timestamp so nanoseconds survive a round trip. detect_config and import_from_path also accept an ISO 8601 string, parsed with pd.Timestamp.
TimeFormat gains UNSPECIFIED, matching the proto's TIME_FORMAT_UNSPECIFIED. A CSV DetectConfig response without a detected format no longer raises; detect_config applies the caller's time_format or ABSOLUTE_UNIX_NANOSECONDS instead. Importing with an unspecified format raises the existing "format must be set" errors.
Drop UNSPECIFIED from the public TimeFormat enum. CsvImportConfig maps the proto's TIME_FORMAT_UNSPECIFIED to None, matching Parquet, TDMS, and HDF5. TimeColumnBase.format now defaults to None, so CSV and Parquet time columns share the same rule: optional at construction, required at import.
…nto import configs
Summary
Flat dataset Parquet config detection fails for wide files (many columns and/or long column names) because the client sends the Parquet footer to the server's
DetectConfigendpoint, and footers over the server's 20 MiB gRPC receive limit are rejected withRESOURCE_EXHAUSTED.This PR detects
PARQUET_FLATDATASETconfigs on the client from the footer schema with pyarrow, the same way HDF5, TDMS, ULog, and MCAP configs are detected. Single channel per row Parquet still uses server detection.detect_configandimport_from_pathnow also acceptrelative_start_timeso files with relative time can be detected and imported without editing the config.Also refactored by moving helper functions either into
_internalmodule or moved them intoImportConfigas methods.Dependency change: flat dataset Parquet detection now requires pyarrow. Install it with
pip install sift-stack-py[parquet].Validation
Recreation
detect_configon a Parquet file with 20,000 columns and long column names:Before this change:
After this change:
Behavior validation
_tests/_internal/test_parquet.py.