Add support for setting retention period using RETAIN syntax - #348
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for setting retention periods in VACUUM commands using the RETAIN num_seconds syntax, while removing the previous environment variable-based configuration approach. The changes enable users to specify retention periods directly in SQL rather than through server configuration.
Key changes:
- Extended the VACUUM SQL parser to support optional
RETAIN num_secondssyntax - Updated all vacuum operations to accept an optional retention period parameter
- Removed environment variable and configuration-based retention period settings
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/modelardb_types/src/flight/protocol.proto | Removed retention_period_in_seconds from Configuration protobuf |
| crates/modelardb_storage/src/parser.rs | Extended VACUUM parser to support RETAIN syntax with validation |
| crates/modelardb_storage/src/delta_lake.rs | Updated vacuum_table to accept optional retention period |
| crates/modelardb_server/tests/integration_test.rs | Updated tests to use new VACUUM syntax instead of configuration |
| crates/modelardb_server/src/remote.rs | Updated FlightService to pass retention period from parsed SQL |
| crates/modelardb_server/src/context.rs | Updated vacuum_table method signature and removed configuration usage |
| crates/modelardb_server/src/configuration.rs | Removed retention period fields and methods from ConfigurationManager |
| crates/modelardb_manager/src/remote.rs | Updated manager to handle new vacuum syntax and parameter passing |
| crates/modelardb_embedded/src/operations/mod.rs | Updated Operations trait vacuum method signature |
| crates/modelardb_embedded/src/operations/data_folder.rs | Removed environment variable usage, updated vacuum implementation |
| crates/modelardb_embedded/src/operations/client.rs | Updated client vacuum to construct SQL with RETAIN syntax |
| crates/modelardb_embedded/src/capi.rs | Updated C API to accept retention period parameter |
| crates/modelardb_embedded/bindings/python/tests/test_operations.py | Updated Python tests to use new vacuum API |
| crates/modelardb_embedded/bindings/python/modelardb/operations.py | Updated Python bindings to support retention period parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
skejserjensen
self-requested a review
September 7, 2025 07:42
skejserjensen
requested changes
Sep 8, 2025
skejserjensen
approved these changes
Sep 8, 2025
chrthomsen
approved these changes
Sep 9, 2025
Closed
9 tasks
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 #343 by changing the parser to support the syntax
VACUUM table_name RETAIN num_seconds. Note thatRETAINis optional.The PR also removes the previous method for setting the retention period which was through the environment variable
MODELARDBD_RETENTION_PERIOD_IN_SECONDSand the configurationretention_period_in_seconds. This method was removed to avoid confusion between the two ways to set the retention period and since the setting was also only supposed to be used in the server (modelardbd). It could have been kept in the server but this might cause confusion on how it is handled in the manager andOperationsAPI.