Add lossless error bound enum variant - #350
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds a Lossless variant to the ErrorBound enum to represent lossless storage of time series values. The absolute and relative error bounds have been updated to no longer allow zero values (only positive values are now permitted), as lossless storage is now explicitly represented by the new Lossless variant.
- Added
Losslessvariant toErrorBoundenum for explicit lossless compression - Changed absolute and relative error bounds to require positive values (no longer allow zero)
- Fixed a bug that allowed unsupported data types in time series table tag columns
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/modelardb_types/src/types.rs | Added Lossless variant to ErrorBound enum and updated validation logic |
| crates/modelardb_types/src/flight/protocol.proto | Added LOSSLESS enum value to protobuf error bound type |
| crates/modelardb_types/src/flight/mod.rs | Updated encoding/decoding logic to handle lossless error bounds |
| crates/modelardb_storage/src/parser.rs | Updated SQL parser to use lossless error bounds as default |
| crates/modelardb_storage/src/metadata/table_metadata_manager.rs | Updated metadata handling for lossless error bounds |
| crates/modelardb_embedded/src/operations/mod.rs | Updated operations to use lossless error bounds as default |
| crates/modelardb_embedded/src/operations/data_folder.rs | Updated data folder operations for lossless error bounds |
| crates/modelardb_embedded/src/capi.rs | Updated C API documentation for lossless error bounds |
| crates/modelardb_embedded/bindings/python/tests/test_error_bound.py | Updated Python tests to reflect new validation rules |
| crates/modelardb_embedded/bindings/python/modelardb/table.py | Updated Python table documentation |
| crates/modelardb_embedded/bindings/python/modelardb/error_bound.py | Updated Python error bound validation |
| crates/modelardb_compression/src/types.rs | Updated compression tests to use lossless error bounds |
| crates/modelardb_compression/src/models/swing.rs | Updated Swing model tests to use lossless error bounds |
| crates/modelardb_compression/src/models/pmc_mean.rs | Updated PMC-Mean model tests to use lossless error bounds |
| crates/modelardb_compression/src/models/mod.rs | Updated model logic to handle lossless error bounds |
| crates/modelardb_compression/src/models/macaque_v.rs | Updated MacaqueV model tests to use lossless error bounds |
| crates/modelardb_compression/src/compression.rs | Updated compression tests to use lossless error bounds |
| crates/modelardb_test/src/table.rs | Updated test utilities to use lossless error bounds |
Comments suppressed due to low confidence (1)
crates/modelardb_types/src/types.rs:1
- The documentation comment lists conditions that return
ModelarDbTypesError, but the newly added validation for unsupported data types (line 106) is too generic. It should specify which data types are supported: timestamp columns must be i64 microseconds, field columns must be f32, and tag columns must be Utf8 strings.
/* Copyright 2022 The ModelarDB Contributors
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
skejserjensen
approved these changes
Sep 9, 2025
chrthomsen
approved these changes
Sep 10, 2025
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 #345 by adding a
Losslessvariant to theErrorBoundenum. The absolute and relative error bounds have also been changed both in the Rust API and in the Python bindings to no longer allow a value of 0.When fixing some unit tests, a bug was found that allowed
TimeSeriesTableMetadatato be created with unsupported types for specifically tag columns. This bug has been fixed and the test that checked for the issue has been fixed.