From ff1cb271d20be00939b1fcc8c3e0f1dbf9b099f0 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Fri, 5 Sep 2025 22:41:34 +0200 Subject: [PATCH 01/26] If the next token is not EOF or RETAIN, attempt to parse table names --- crates/modelardb_storage/src/parser.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index c5da7a146..45908a0e7 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -507,7 +507,12 @@ impl ModelarDbDialect { let mut table_names = vec![]; - if Token::EOF != parser.peek_nth_token(0).token { + + // If the next token is not EOF or RETAIN, attempt to parse table names. + if Token::EOF != parser.peek_nth_token(0).token + && let Token::Word(word) = parser.peek_nth_token(0).token + && word.keyword != Keyword::RETAIN + { loop { match self.parse_word_value(parser) { Ok(table_name) => { From 7c63d188f5286cd06d8898295b37f03dfea202fc Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Fri, 5 Sep 2025 23:11:00 +0200 Subject: [PATCH 02/26] If the next token is RETAIN, attempt to parse the retention period in seconds --- crates/modelardb_storage/src/parser.rs | 31 +++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 45908a0e7..0f01ea5a5 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -508,9 +508,8 @@ impl ModelarDbDialect { let mut table_names = vec![]; - // If the next token is not EOF or RETAIN, attempt to parse table names. - if Token::EOF != parser.peek_nth_token(0).token - && let Token::Word(word) = parser.peek_nth_token(0).token + // If the next token is a word that is not RETAIN, attempt to parse table names. + if let Token::Word(word) = parser.peek_nth_token(0).token && word.keyword != Keyword::RETAIN { loop { @@ -528,11 +527,37 @@ impl ModelarDbDialect { } } + // If the next token is RETAIN, attempt to parse the retention period in seconds. + let maybe_retention_period = if let Token::Word(word) = parser.peek_nth_token(0).token + && word.keyword == Keyword::RETAIN + { + parser.expect_keyword(Keyword::RETAIN)?; + Some(self.parse_unsigned_literal_u64(parser)?) + } else { + None + }; + + println!("Retention period: {:?}", maybe_retention_period); + // Return Statement::ShowVariable as a substitute for Vacuum. Ok(Statement::ShowVariable { variable: table_names, }) } + + /// Return its value as a [`u64`] if the next [`Token`] is a [`Token::Number`], otherwise a + /// [`ParserError`] is returned. + fn parse_unsigned_literal_u64(&self, parser: &mut Parser) -> StdResult { + let token_with_location = parser.next_token(); + match token_with_location.token { + Token::Number(maybe_u64, _) => maybe_u64.parse::().map_err(|error| { + ParserError::ParserError(format!( + "Failed to parse '{maybe_u64}' into a u64 due to: {error}" + )) + }), + _ => parser.expected("literal integer", token_with_location), + } + } } /// Create a [`Setting`] with `key`, `quote_style`, and `value`. From 7b5d0e7b38d7a325665b6dbafd8aebe64034f771 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 12:45:34 +0200 Subject: [PATCH 03/26] Use NOTIFY instead of ShowVariable for VACUUM --- crates/modelardb_storage/src/parser.rs | 29 +++++++++++++------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 0f01ea5a5..c557d4cc7 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -173,7 +173,7 @@ pub fn tokenize_and_parse_sql_expression( /// SQL dialect that extends `sqlparsers's` [`GenericDialect`] with support for parsing CREATE TIME /// SERIES TABLE table_name DDL statements, INCLUDE 'address'\[, 'address'\]+ DQL statements, and -/// VACUUM \[table_name\[, table_name\]+\] statements. +/// VACUUM \[table_name\[, table_name\]+\] \[RETAIN num_seconds\] statements. #[derive(Debug)] struct ModelarDbDialect { /// Dialect to use for identifying identifiers. @@ -497,17 +497,17 @@ impl ModelarDbDialect { } } - /// Parse VACUUM \[table_name\[, table_name\]+\] to a [`Statement::ShowVariable`] with the - /// table names in the `variable` field. Note that [`Statement::ShowVariable`] is used since - /// [`Statement`] does not have a `Vacuum` variant. A [`ParserError`] is returned if VACUUM is - /// not the first word or the table names cannot be extracted. + /// Parse VACUUM \[table_name\[, table_name\]+\] \[RETAIN num_seconds\] to a [`Statement::NOTIFY`] + /// with the table names in the `channel` field and the optional retention period in the `payload` + /// field. Note that [`Statement::NOTIFY`] is used since [`Statement`] does not have a `Vacuum` + /// variant. A [`ParserError`] is returned if VACUUM is not the first word, the table names + /// cannot be extracted, or the retention period is not a valid positive integer. fn parse_vacuum(&self, parser: &mut Parser) -> StdResult { // VACUUM. parser.expect_keyword(Keyword::VACUUM)?; let mut table_names = vec![]; - // If the next token is a word that is not RETAIN, attempt to parse table names. if let Token::Word(word) = parser.peek_nth_token(0).token && word.keyword != Keyword::RETAIN @@ -515,7 +515,7 @@ impl ModelarDbDialect { loop { match self.parse_word_value(parser) { Ok(table_name) => { - table_names.push(Ident::new(table_name)); + table_names.push(table_name); if Token::Comma == parser.peek_nth_token(0).token { parser.next_token(); } else { @@ -537,11 +537,10 @@ impl ModelarDbDialect { None }; - println!("Retention period: {:?}", maybe_retention_period); - - // Return Statement::ShowVariable as a substitute for Vacuum. - Ok(Statement::ShowVariable { - variable: table_names, + // Return Statement::NOTIFY as a substitute for Vacuum. + Ok(Statement::NOTIFY { + channel: Ident::new(table_names.join(";")), + payload: maybe_retention_period.map(|period| period.to_string()), }) } @@ -588,9 +587,9 @@ impl Dialect for ModelarDbDialect { /// as a CREATE TIME SERIES TABLE DDL statement. If not, check if the next token is INCLUDE, if so, /// attempt to parse the token stream as an INCLUDE 'address'\[, 'address'\]+ DQL statement. /// If not, check if the next token is VACUUM, if so, attempt to parse the token stream as a - /// VACUUM \[table_name\[, table_name\]+\] statement. If all checks fail, [`None`] is returned - /// so [`sqlparser`] uses its parsing methods for all other statements. If parsing succeeds, a - /// [`Statement`] is returned, and if not, a [`ParserError`] is returned. + /// VACUUM \[table_name\[, table_name\]+\] \[RETAIN num_seconds\] statement. If all checks fail, + /// [`None`] is returned so [`sqlparser`] uses its parsing methods for all other statements. + /// If parsing succeeds, a [`Statement`] is returned, and if not, a [`ParserError`] is returned. fn parse_statement(&self, parser: &mut Parser) -> Option> { if self.next_tokens_are_create_time_series_table(parser) { Some(self.parse_create_time_series_table(parser)) From 02ab1353baac8a2850d4baa82e2f9c34f9ac0ad1 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 12:50:59 +0200 Subject: [PATCH 04/26] Update ModelarDBStatement::Vacuum to match changes to parsing --- crates/modelardb_storage/src/parser.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index c557d4cc7..61428ed1f 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -68,7 +68,7 @@ pub enum ModelarDbStatement { /// TRUNCATE TABLE. TruncateTable(Vec), /// VACUUM. - Vacuum(Vec), + Vacuum(Vec, Option), } /// Tokenizes and parses the SQL statement in `sql` and returns its parsed representation in the form @@ -131,10 +131,11 @@ pub fn tokenize_and_parse_sql_statement(sql_statement: &str) -> Result Ok(ModelarDbStatement::Vacuum( - variable.into_iter().map(|ident| ident.value).collect(), + Statement::NOTIFY { channel, payload } => Ok(ModelarDbStatement::Vacuum( + channel.value.split_terminator(';').map(|s| s.to_owned()).collect(), + payload.and_then(|p| p.parse::().ok()), )), Statement::Explain { .. } => Ok(ModelarDbStatement::Statement(statement)), Statement::Query(ref boxed_query) => { @@ -1690,21 +1691,22 @@ mod tests { #[test] fn test_tokenize_and_parse_vacuum_all_tables() { - let table_names = parse_vacuum_and_extract_table_names("VACUUM"); + let (table_names, _) = parse_vacuum_and_extract_table_names("VACUUM"); assert!(table_names.is_empty()); } #[test] fn test_tokenize_and_parse_vacuum_single_table() { - let table_names = parse_vacuum_and_extract_table_names("VACUUM table_name"); + let (table_names, _) = parse_vacuum_and_extract_table_names("VACUUM table_name"); assert_eq!(table_names, vec!["table_name".to_owned()]); } #[test] fn test_tokenize_and_parse_vacuum_multiple_tables() { - let table_names = parse_vacuum_and_extract_table_names("VACUUM table_name_1, table_name_2"); + let (table_names, _) = + parse_vacuum_and_extract_table_names("VACUUM table_name_1, table_name_2"); assert_eq!( table_names, @@ -1712,11 +1714,13 @@ mod tests { ); } - fn parse_vacuum_and_extract_table_names(sql_statement: &str) -> Vec { + fn parse_vacuum_and_extract_table_names(sql_statement: &str) -> (Vec, Option) { let modelardb_statement = tokenize_and_parse_sql_statement(sql_statement).unwrap(); match modelardb_statement { - ModelarDbStatement::Vacuum(table_names) => table_names, + ModelarDbStatement::Vacuum(table_names, maybe_retention_period) => { + (table_names, maybe_retention_period) + } _ => panic!("Expected ModelarDbStatement::Vacuum."), } } From fc814336925c2851441bcb5cee66f7ed2f76d73d Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 15:20:48 +0200 Subject: [PATCH 05/26] Add tests for new RETAIN syntax --- crates/modelardb_storage/src/parser.rs | 89 +++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 3 deletions(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 61428ed1f..13227e3ee 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -1691,27 +1691,52 @@ mod tests { #[test] fn test_tokenize_and_parse_vacuum_all_tables() { - let (table_names, _) = parse_vacuum_and_extract_table_names("VACUUM"); + let (table_names, maybe_retention_period) = parse_vacuum_and_extract_table_names("VACUUM"); assert!(table_names.is_empty()); + assert!(maybe_retention_period.is_none()); } #[test] fn test_tokenize_and_parse_vacuum_single_table() { - let (table_names, _) = parse_vacuum_and_extract_table_names("VACUUM table_name"); + let (table_names, maybe_retention_period) = + parse_vacuum_and_extract_table_names("VACUUM table_name"); assert_eq!(table_names, vec!["table_name".to_owned()]); + assert!(maybe_retention_period.is_none()); } #[test] fn test_tokenize_and_parse_vacuum_multiple_tables() { - let (table_names, _) = + let (table_names, maybe_retention_period) = parse_vacuum_and_extract_table_names("VACUUM table_name_1, table_name_2"); assert_eq!( table_names, vec!["table_name_1".to_owned(), "table_name_2".to_owned()] ); + assert!(maybe_retention_period.is_none()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_with_retention_period() { + let (table_names, maybe_retention_period) = + parse_vacuum_and_extract_table_names("VACUUM RETAIN 30"); + + assert!(table_names.is_empty()); + assert_eq!(maybe_retention_period, Some(30)); + } + + #[test] + fn test_tokenize_and_parse_vacuum_multiple_tables_with_retention_period() { + let (table_names, maybe_retention_period) = + parse_vacuum_and_extract_table_names("VACUUM table_name_1, table_name_2 RETAIN 30"); + + assert_eq!( + table_names, + vec!["table_name_1".to_owned(), "table_name_2".to_owned()] + ); + assert_eq!(maybe_retention_period, Some(30)); } fn parse_vacuum_and_extract_table_names(sql_statement: &str) -> (Vec, Option) { @@ -1735,8 +1760,66 @@ mod tests { assert!(tokenize_and_parse_sql_statement("VACUUM ,table_name").is_err()); } + #[test] + fn test_tokenize_and_parse_vacuum_only_comma() { + assert!(tokenize_and_parse_sql_statement("VACUUM,").is_err()); + } + #[test] fn test_tokenize_and_parse_vacuum_quoted_table_name() { assert!(tokenize_and_parse_sql_statement("VACUUM 'table_name'").is_err()); } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_without_number() { + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_number_without_retain() { + assert!(tokenize_and_parse_sql_statement("VACUUM 30").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_with_float() { + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN 30.5").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_with_non_numeric() { + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN thirty").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_with_negative() { + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN -5").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_with_u64_max_plus_one() { + let max_plus_one = u64::MAX as u128 + 1; + assert!( + tokenize_and_parse_sql_statement(&format!("VACUUM RETAIN {}", max_plus_one)).is_err() + ); + } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_twice() { + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN 30 RETAIN 30").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_multiple_tables_retain_without_number() { + assert!(tokenize_and_parse_sql_statement("VACUUM table_1, table_2 RETAIN").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_tables_and_retain_mixed() { + assert!(tokenize_and_parse_sql_statement("VACUUM table_1, RETAIN 30, table_2").is_err()); + } + + #[test] + fn test_tokenize_and_parse_vacuum_retain_first() { + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN 30 table_1, table_2").is_err()); + } } From b380c407bedb2a9d869f97e95dc88be09fb37573 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:13:10 +0200 Subject: [PATCH 06/26] Pass retention perion down --- crates/modelardb_manager/src/remote.rs | 19 +++++++++++++++---- crates/modelardb_server/src/remote.rs | 2 +- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/crates/modelardb_manager/src/remote.rs b/crates/modelardb_manager/src/remote.rs index c647b2d8d..01a9508d3 100644 --- a/crates/modelardb_manager/src/remote.rs +++ b/crates/modelardb_manager/src/remote.rs @@ -337,7 +337,11 @@ impl FlightServiceHandler { /// Vacuum the table in the remote data folder and at each node controlled by the manager. If /// the table does not exist or the table cannot be vacuumed in the remote data folder /// and at each node, return [`Status`]. - async fn vacuum_cluster_table(&self, table_name: &str) -> StdResult<(), Status> { + async fn vacuum_cluster_table( + &self, + table_name: &str, + maybe_retention_period: Option, + ) -> StdResult<(), Status> { let retention_period_in_seconds = env::var("MODELARDBD_RETENTION_PERIOD_IN_SECONDS") .map_or(60 * 60 * 24 * 7, |value| value.parse().unwrap()); @@ -350,11 +354,17 @@ impl FlightServiceHandler { .map_err(error_to_status_internal)?; // Vacuum the table in the nodes controlled by the manager. + let vacuum_sql = if let Some(retention_period) = maybe_retention_period { + format!("VACUUM {table_name} RETAIN {retention_period}") + } else { + format!("VACUUM {table_name}") + }; + self.context .cluster .read() .await - .cluster_do_get(&format!("VACUUM {table_name}"), &self.context.key) + .cluster_do_get(&vacuum_sql, &self.context.key) .await .map_err(error_to_status_internal)?; @@ -524,7 +534,7 @@ impl FlightService for FlightServiceHandler { self.drop_cluster_table(&table_name).await?; } } - ModelarDbStatement::Vacuum(mut table_names) => { + ModelarDbStatement::Vacuum(mut table_names, maybe_retention_period) => { // Vacuum all tables if no table names are provided. if table_names.is_empty() { table_names = self @@ -538,7 +548,8 @@ impl FlightService for FlightServiceHandler { } for table_name in table_names { - self.vacuum_cluster_table(&table_name).await?; + self.vacuum_cluster_table(&table_name, maybe_retention_period) + .await?; } } // .. is not used so a compile error is raised if a new ModelarDbStatement is added. diff --git a/crates/modelardb_server/src/remote.rs b/crates/modelardb_server/src/remote.rs index 5579a7580..51f0d2679 100644 --- a/crates/modelardb_server/src/remote.rs +++ b/crates/modelardb_server/src/remote.rs @@ -516,7 +516,7 @@ impl FlightService for FlightServiceHandler { Ok(empty_record_batch_stream()) } - ModelarDbStatement::Vacuum(mut table_names) => { + ModelarDbStatement::Vacuum(mut table_names, maybe_retention_period) => { // Vacuum all tables if no table names are provided. if table_names.is_empty() { table_names = self From f32a44a56660740825b0ed8d72daba58c2bff712 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:29:06 +0200 Subject: [PATCH 07/26] Use Option for retention period instead of u64 --- crates/modelardb_storage/src/delta_lake.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index 8d8662903..79e3cb1bf 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -467,18 +467,21 @@ impl DeltaLake { } /// Vacuum the Delta Lake table with `table_name` by deleting all files that are older than - /// `retention_period_in_seconds` seconds. If the retention period is out of bounds or the - /// files could not be deleted, a [`ModelarDbStorageError`] is returned. + /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the + /// default retention period of 7 days is used. If the retention period is larger than i64::MAX + /// milliseconds or the files could not be deleted, a [`ModelarDbStorageError`] is returned. pub async fn vacuum_table( &self, table_name: &str, - retention_period_in_seconds: usize, + maybe_retention_period_in_seconds: Option, ) -> Result<()> { let delta_table_ops = self.delta_ops(table_name).await?; - let retention_period = TimeDelta::new(retention_period_in_seconds as i64, 0).ok_or( + let retention_period_in_seconds = + maybe_retention_period_in_seconds.unwrap_or(60 * 60 * 24 * 7); + let retention_period = TimeDelta::new(retention_period_in_seconds, 0).ok_or( ModelarDbStorageError::InvalidArgument(format!( - "Retention period of {retention_period_in_seconds} seconds is out of bounds." + "Retention period of {retention_period_in_seconds} seconds is larger than i64::MAX milliseconds." )), )?; From 85155631718a939d3d02c1ef8a02f4f06b212fc9 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:38:10 +0200 Subject: [PATCH 08/26] Remove retention_period_in_seconds configuration --- crates/modelardb_server/src/configuration.rs | 47 ------------------- crates/modelardb_server/src/remote.rs | 7 --- .../tests/integration_test.rs | 12 ----- .../modelardb_types/src/flight/protocol.proto | 10 ++-- 4 files changed, 3 insertions(+), 73 deletions(-) diff --git a/crates/modelardb_server/src/configuration.rs b/crates/modelardb_server/src/configuration.rs index 4663152bc..b85643f9d 100644 --- a/crates/modelardb_server/src/configuration.rs +++ b/crates/modelardb_server/src/configuration.rs @@ -45,8 +45,6 @@ pub struct ConfigurationManager { /// The number of seconds between each transfer of data to the remote object store. If [`None`], /// data is not transferred based on time. transfer_time_in_seconds: Option, - /// The number of seconds to retain deleted data in storage before it can be removed by vacuum. - retention_period_in_seconds: usize, /// Number of threads to allocate for converting multivariate time series to univariate /// time series. pub(crate) ingestion_threads: usize, @@ -76,9 +74,6 @@ impl ConfigurationManager { let transfer_time_in_seconds = env::var("MODELARDBD_TRANSFER_TIME_IN_SECONDS") .map_or(None, |value| Some(value.parse().unwrap())); - let retention_period_in_seconds = env::var("MODELARDBD_RETENTION_PERIOD_IN_SECONDS") - .map_or(60 * 60 * 24 * 7, |value| value.parse().unwrap()); - Self { cluster_mode, multivariate_reserved_memory_in_bytes, @@ -86,7 +81,6 @@ impl ConfigurationManager { compressed_reserved_memory_in_bytes, transfer_batch_size_in_bytes, transfer_time_in_seconds, - retention_period_in_seconds, // TODO: Add support for running multiple threads per component. The individual // components in the storage engine have not been validated with multiple threads, e.g., // UncompressedDataManager may have race conditions finishing buffers if multiple @@ -223,18 +217,6 @@ impl ConfigurationManager { Ok(()) } - pub(crate) fn retention_period_in_seconds(&self) -> usize { - self.retention_period_in_seconds - } - - /// Set the new value for the retention period in seconds. - pub(crate) fn set_retention_period_in_seconds( - &mut self, - new_retention_period_in_seconds: usize, - ) { - self.retention_period_in_seconds = new_retention_period_in_seconds; - } - /// Encode the current configuration into a [`Configuration`](protocol::Configuration) /// protobuf message and serialize it. pub(crate) fn encode_and_serialize(&self) -> Vec { @@ -246,7 +228,6 @@ impl ConfigurationManager { compressed_reserved_memory_in_bytes: self.compressed_reserved_memory_in_bytes as u64, transfer_batch_size_in_bytes: self.transfer_batch_size_in_bytes.map(|v| v as u64), transfer_time_in_seconds: self.transfer_time_in_seconds.map(|v| v as u64), - retention_period_in_seconds: self.retention_period_in_seconds as u64, ingestion_threads: self.ingestion_threads as u32, compression_threads: self.compression_threads as u32, writer_threads: self.writer_threads as u32, @@ -421,34 +402,6 @@ mod tests { ); } - #[tokio::test] - async fn test_set_retention_period_in_seconds() { - let temp_dir = tempfile::tempdir().unwrap(); - let (_, configuration_manager) = create_components(&temp_dir).await; - - assert_eq!( - configuration_manager - .read() - .await - .retention_period_in_seconds(), - 60 * 60 * 24 * 7 - ); - - let new_value = 60; - configuration_manager - .write() - .await - .set_retention_period_in_seconds(new_value); - - assert_eq!( - configuration_manager - .read() - .await - .retention_period_in_seconds(), - new_value - ); - } - /// Create a [`StorageEngine`] and a [`ConfigurationManager`]. async fn create_components( temp_dir: &TempDir, diff --git a/crates/modelardb_server/src/remote.rs b/crates/modelardb_server/src/remote.rs index 51f0d2679..3baa773a8 100644 --- a/crates/modelardb_server/src/remote.rs +++ b/crates/modelardb_server/src/remote.rs @@ -770,13 +770,6 @@ impl FlightService for FlightServiceHandler { .await .map_err(error_to_status_internal) } - Ok(protocol::update_configuration::Setting::RetentionPeriodInSeconds) => { - let new_value = new_value.ok_or(invalid_null_error)?; - - configuration_manager.set_retention_period_in_seconds(new_value); - - Ok(()) - } _ => Err(Status::unimplemented(format!( "{setting} is not an updatable setting in the server configuration." ))), diff --git a/crates/modelardb_server/tests/integration_test.rs b/crates/modelardb_server/tests/integration_test.rs index 623385100..c51aa1e95 100644 --- a/crates/modelardb_server/tests/integration_test.rs +++ b/crates/modelardb_server/tests/integration_test.rs @@ -1273,7 +1273,6 @@ async fn test_can_get_configuration() { Some(64 * 1024 * 1024) ); assert_eq!(configuration.transfer_time_in_seconds, None); - assert_eq!(configuration.retention_period_in_seconds, 60 * 60 * 24 * 7); assert_eq!(configuration.ingestion_threads, 1); assert_eq!(configuration.compression_threads, 1); assert_eq!(configuration.writer_threads, 1); @@ -1315,16 +1314,6 @@ async fn test_can_update_compressed_reserved_memory_in_bytes() { assert_eq!(updated_configuration.compressed_reserved_memory_in_bytes, 1); } -#[tokio::test] -async fn test_can_update_retention_period_in_seconds() { - let updated_configuration = update_and_get_configuration( - protocol::update_configuration::Setting::RetentionPeriodInSeconds as i32, - ) - .await; - - assert_eq!(updated_configuration.retention_period_in_seconds, 1); -} - async fn update_and_get_configuration(setting: i32) -> protocol::Configuration { let mut test_context = TestContext::new().await; test_context @@ -1376,7 +1365,6 @@ async fn test_cannot_update_non_nullable_setting_with_null_value() { protocol::update_configuration::Setting::MultivariateReservedMemoryInBytes as i32, protocol::update_configuration::Setting::UncompressedReservedMemoryInBytes as i32, protocol::update_configuration::Setting::CompressedReservedMemoryInBytes as i32, - protocol::update_configuration::Setting::RetentionPeriodInSeconds as i32, ] { update_configuration_and_assert_error( setting, diff --git a/crates/modelardb_types/src/flight/protocol.proto b/crates/modelardb_types/src/flight/protocol.proto index 5ddec3592..8d2babd82 100644 --- a/crates/modelardb_types/src/flight/protocol.proto +++ b/crates/modelardb_types/src/flight/protocol.proto @@ -107,17 +107,14 @@ message Configuration { // The number of seconds between each transfer of data to the remote object store. optional uint64 transfer_time_in_seconds = 5; - // The number of seconds to retain deleted data in storage before it can be removed by vacuum. - uint64 retention_period_in_seconds = 6; - // Number of threads to allocate for converting multivariate time series to univariate time series. - uint32 ingestion_threads = 7; + uint32 ingestion_threads = 6; // Number of threads to allocate for compressing univariate time series to segments. - uint32 compression_threads = 8; + uint32 compression_threads = 7; // Number of threads to allocate for writing segments to a local and/or remote data folder. - uint32 writer_threads = 9; + uint32 writer_threads = 8; } // Request to update the configuration of a ModelarDB node. @@ -128,7 +125,6 @@ message UpdateConfiguration { COMPRESSED_RESERVED_MEMORY_IN_BYTES = 2; TRANSFER_BATCH_SIZE_IN_BYTES = 3; TRANSFER_TIME_IN_SECONDS = 4; - RETENTION_PERIOD_IN_SECONDS = 5; } // Setting to update in the configuration. From 869747f6ee9f2453f6ba7fd7df67c5aff82ea4d4 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:41:16 +0200 Subject: [PATCH 09/26] Rename to _retention_period_in_seconds --- crates/modelardb_storage/src/parser.rs | 30 ++++++++++++++------------ 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 13227e3ee..d0c6a70ac 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -529,7 +529,8 @@ impl ModelarDbDialect { } // If the next token is RETAIN, attempt to parse the retention period in seconds. - let maybe_retention_period = if let Token::Word(word) = parser.peek_nth_token(0).token + let maybe_retention_period_in_seconds = if let Token::Word(word) = + parser.peek_nth_token(0).token && word.keyword == Keyword::RETAIN { parser.expect_keyword(Keyword::RETAIN)?; @@ -541,7 +542,7 @@ impl ModelarDbDialect { // Return Statement::NOTIFY as a substitute for Vacuum. Ok(Statement::NOTIFY { channel: Ident::new(table_names.join(";")), - payload: maybe_retention_period.map(|period| period.to_string()), + payload: maybe_retention_period_in_seconds.map(|period| period.to_string()), }) } @@ -1691,60 +1692,61 @@ mod tests { #[test] fn test_tokenize_and_parse_vacuum_all_tables() { - let (table_names, maybe_retention_period) = parse_vacuum_and_extract_table_names("VACUUM"); + let (table_names, maybe_retention_period_in_seconds) = + parse_vacuum_and_extract_table_names("VACUUM"); assert!(table_names.is_empty()); - assert!(maybe_retention_period.is_none()); + assert!(maybe_retention_period_in_seconds.is_none()); } #[test] fn test_tokenize_and_parse_vacuum_single_table() { - let (table_names, maybe_retention_period) = + let (table_names, maybe_retention_period_in_seconds) = parse_vacuum_and_extract_table_names("VACUUM table_name"); assert_eq!(table_names, vec!["table_name".to_owned()]); - assert!(maybe_retention_period.is_none()); + assert!(maybe_retention_period_in_seconds.is_none()); } #[test] fn test_tokenize_and_parse_vacuum_multiple_tables() { - let (table_names, maybe_retention_period) = + let (table_names, maybe_retention_period_in_seconds) = parse_vacuum_and_extract_table_names("VACUUM table_name_1, table_name_2"); assert_eq!( table_names, vec!["table_name_1".to_owned(), "table_name_2".to_owned()] ); - assert!(maybe_retention_period.is_none()); + assert!(maybe_retention_period_in_seconds.is_none()); } #[test] fn test_tokenize_and_parse_vacuum_with_retention_period() { - let (table_names, maybe_retention_period) = + let (table_names, maybe_retention_period_in_seconds) = parse_vacuum_and_extract_table_names("VACUUM RETAIN 30"); assert!(table_names.is_empty()); - assert_eq!(maybe_retention_period, Some(30)); + assert_eq!(maybe_retention_period_in_seconds, Some(30)); } #[test] fn test_tokenize_and_parse_vacuum_multiple_tables_with_retention_period() { - let (table_names, maybe_retention_period) = + let (table_names, maybe_retention_period_in_seconds) = parse_vacuum_and_extract_table_names("VACUUM table_name_1, table_name_2 RETAIN 30"); assert_eq!( table_names, vec!["table_name_1".to_owned(), "table_name_2".to_owned()] ); - assert_eq!(maybe_retention_period, Some(30)); + assert_eq!(maybe_retention_period_in_seconds, Some(30)); } fn parse_vacuum_and_extract_table_names(sql_statement: &str) -> (Vec, Option) { let modelardb_statement = tokenize_and_parse_sql_statement(sql_statement).unwrap(); match modelardb_statement { - ModelarDbStatement::Vacuum(table_names, maybe_retention_period) => { - (table_names, maybe_retention_period) + ModelarDbStatement::Vacuum(table_names, maybe_retention_period_in_seconds) => { + (table_names, maybe_retention_period_in_seconds) } _ => panic!("Expected ModelarDbStatement::Vacuum."), } From e779f9f4be9ce681e5647b6e5908f4f69b6e59b7 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:48:42 +0200 Subject: [PATCH 10/26] No longer use MODELARDBD_RETENTION_PERIOD_IN_SECONDS in manager --- crates/modelardb_manager/src/remote.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/modelardb_manager/src/remote.rs b/crates/modelardb_manager/src/remote.rs index 01a9508d3..e487b1375 100644 --- a/crates/modelardb_manager/src/remote.rs +++ b/crates/modelardb_manager/src/remote.rs @@ -21,8 +21,8 @@ use std::error::Error; use std::net::SocketAddr; use std::pin::Pin; use std::result::Result as StdResult; +use std::str; use std::sync::Arc; -use std::{env, str}; use arrow::datatypes::Schema; use arrow::ipc::writer::IpcWriteOptions; @@ -340,21 +340,18 @@ impl FlightServiceHandler { async fn vacuum_cluster_table( &self, table_name: &str, - maybe_retention_period: Option, + maybe_retention_period_in_seconds: Option, ) -> StdResult<(), Status> { - let retention_period_in_seconds = env::var("MODELARDBD_RETENTION_PERIOD_IN_SECONDS") - .map_or(60 * 60 * 24 * 7, |value| value.parse().unwrap()); - // Vacuum the table in the remote data folder Delta lake. self.context .remote_data_folder .delta_lake - .vacuum_table(table_name, retention_period_in_seconds) + .vacuum_table(table_name, maybe_retention_period_in_seconds) .await .map_err(error_to_status_internal)?; // Vacuum the table in the nodes controlled by the manager. - let vacuum_sql = if let Some(retention_period) = maybe_retention_period { + let vacuum_sql = if let Some(retention_period) = maybe_retention_period_in_seconds { format!("VACUUM {table_name} RETAIN {retention_period}") } else { format!("VACUUM {table_name}") @@ -534,7 +531,7 @@ impl FlightService for FlightServiceHandler { self.drop_cluster_table(&table_name).await?; } } - ModelarDbStatement::Vacuum(mut table_names, maybe_retention_period) => { + ModelarDbStatement::Vacuum(mut table_names, maybe_retention_period_in_seconds) => { // Vacuum all tables if no table names are provided. if table_names.is_empty() { table_names = self @@ -548,7 +545,7 @@ impl FlightService for FlightServiceHandler { } for table_name in table_names { - self.vacuum_cluster_table(&table_name, maybe_retention_period) + self.vacuum_cluster_table(&table_name, maybe_retention_period_in_seconds) .await?; } } From 3e49167eb05e80af55b903d84aa7cf722b1bd3db Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:52:52 +0200 Subject: [PATCH 11/26] Use retention period from statement in server context --- crates/modelardb_server/src/context.rs | 63 ++++++++++++++++---------- crates/modelardb_server/src/remote.rs | 4 +- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/crates/modelardb_server/src/context.rs b/crates/modelardb_server/src/context.rs index 97bea4ca1..b34562fa5 100644 --- a/crates/modelardb_server/src/context.rs +++ b/crates/modelardb_server/src/context.rs @@ -331,19 +331,19 @@ impl Context { Ok(()) } - /// Vacuum the table with `table_name` if it exists. If the table does not exist or if it could - /// not be vacuumed, [`ModelarDbServerError`] is returned. - pub async fn vacuum_table(&self, table_name: &str) -> Result<()> { - let retention_period_in_seconds = self - .configuration_manager - .read() - .await - .retention_period_in_seconds(); - + /// Vacuum the table with `table_name` if it exists. If a retention period is not given, the + /// default retention period of 7 days is used. If the retention period is larger than i64::MAX + /// milliseconds, the table does not exist, or if it could not be vacuumed, + /// [`ModelarDbServerError`] is returned. + pub async fn vacuum_table( + &self, + table_name: &str, + maybe_retention_period_in_seconds: Option, + ) -> Result<()> { self.data_folders .local_data_folder .delta_lake - .vacuum_table(table_name, retention_period_in_seconds) + .vacuum_table(table_name, maybe_retention_period_in_seconds) .await?; Ok(()) @@ -756,12 +756,6 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let context = create_context_with_normal_table(&temp_dir).await; - context - .configuration_manager - .write() - .await - .set_retention_period_in_seconds(0); - context.truncate_table(NORMAL_TABLE_NAME).await.unwrap(); // The files should still exist on disk even though they are no longer active. @@ -773,7 +767,10 @@ mod tests { let files = std::fs::read_dir(&table_path).unwrap(); assert_eq!(files.count(), 2); - context.vacuum_table(NORMAL_TABLE_NAME).await.unwrap(); + context + .vacuum_table(NORMAL_TABLE_NAME, Some(0)) + .await + .unwrap(); // Only the _delta_log folder should remain. let files = std::fs::read_dir(&table_path).unwrap(); @@ -808,12 +805,6 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let context = create_context_with_time_series_table(&temp_dir).await; - context - .configuration_manager - .write() - .await - .set_retention_period_in_seconds(0); - context .truncate_table(TIME_SERIES_TABLE_NAME) .await @@ -828,13 +819,30 @@ mod tests { let files = std::fs::read_dir(&column_path).unwrap(); assert_eq!(files.count(), 1); - context.vacuum_table(TIME_SERIES_TABLE_NAME).await.unwrap(); + context + .vacuum_table(TIME_SERIES_TABLE_NAME, Some(0)) + .await + .unwrap(); // No files should remain in the column folder. let files = std::fs::read_dir(&column_path).unwrap(); assert_eq!(files.count(), 0); } + #[tokio::test] + async fn test_vacuum_table_with_out_of_bounds_retention_period() { + let temp_dir = tempfile::tempdir().unwrap(); + let context = create_context_with_time_series_table(&temp_dir).await; + + let retention_period_in_seconds = (i64::MAX / 1000 + 1) as u64; + assert!( + context + .vacuum_table(TIME_SERIES_TABLE_NAME, Some(retention_period_in_seconds)) + .await + .is_err() + ); + } + /// Create a [`Context`] with a time series table named `TIME_SERIES_TABLE_NAME` and write data /// to it. async fn create_context_with_time_series_table(temp_dir: &TempDir) -> Arc { @@ -864,7 +872,12 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let context = create_context(&temp_dir).await; - assert!(context.vacuum_table(TIME_SERIES_TABLE_NAME).await.is_err()); + assert!( + context + .vacuum_table(TIME_SERIES_TABLE_NAME, None) + .await + .is_err() + ); } #[tokio::test] diff --git a/crates/modelardb_server/src/remote.rs b/crates/modelardb_server/src/remote.rs index 3baa773a8..66d509e4b 100644 --- a/crates/modelardb_server/src/remote.rs +++ b/crates/modelardb_server/src/remote.rs @@ -516,7 +516,7 @@ impl FlightService for FlightServiceHandler { Ok(empty_record_batch_stream()) } - ModelarDbStatement::Vacuum(mut table_names, maybe_retention_period) => { + ModelarDbStatement::Vacuum(mut table_names, maybe_retention_period_in_seconds) => { // Vacuum all tables if no table names are provided. if table_names.is_empty() { table_names = self @@ -528,7 +528,7 @@ impl FlightService for FlightServiceHandler { for table_name in table_names { self.context - .vacuum_table(&table_name) + .vacuum_table(&table_name, maybe_retention_period_in_seconds) .await .map_err(error_to_status_invalid_argument)?; } From e3bf972d7a69c94bdeb4a163ead494c1ed716f92 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 19:53:18 +0200 Subject: [PATCH 12/26] Use u64 in parameter for simplicity and to avoid negative values --- crates/modelardb_storage/src/delta_lake.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index 79e3cb1bf..0b915f70f 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -473,13 +473,13 @@ impl DeltaLake { pub async fn vacuum_table( &self, table_name: &str, - maybe_retention_period_in_seconds: Option, + maybe_retention_period_in_seconds: Option, ) -> Result<()> { let delta_table_ops = self.delta_ops(table_name).await?; let retention_period_in_seconds = maybe_retention_period_in_seconds.unwrap_or(60 * 60 * 24 * 7); - let retention_period = TimeDelta::new(retention_period_in_seconds, 0).ok_or( + let retention_period = TimeDelta::new(retention_period_in_seconds as i64, 0).ok_or( ModelarDbStorageError::InvalidArgument(format!( "Retention period of {retention_period_in_seconds} seconds is larger than i64::MAX milliseconds." )), From 9911efd3a9f6e8979ad5f2415f6b7f20d4ab0588 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 20:13:24 +0200 Subject: [PATCH 13/26] Use RETAIN num_seconds in VACUUM integration tests --- .../tests/integration_test.rs | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/crates/modelardb_server/tests/integration_test.rs b/crates/modelardb_server/tests/integration_test.rs index c51aa1e95..d4812921c 100644 --- a/crates/modelardb_server/tests/integration_test.rs +++ b/crates/modelardb_server/tests/integration_test.rs @@ -258,8 +258,15 @@ impl TestContext { async fn vacuum_table( &mut self, table_name: &str, + maybe_retention_period_in_seconds: Option, ) -> Result>, Status> { - let ticket = Ticket::new(format!("VACUUM {table_name}")); + let sql = if let Some(retention_period_in_seconds) = maybe_retention_period_in_seconds { + format!("VACUUM {table_name} RETAIN {retention_period_in_seconds}") + } else { + format!("VACUUM {table_name}") + }; + + let ticket = Ticket::new(sql); self.client.do_get(ticket).await } @@ -695,13 +702,6 @@ async fn test_cannot_truncate_missing_table() { #[tokio::test] async fn test_can_vacuum_normal_table() { let mut test_context = TestContext::new().await; - test_context - .update_configuration( - protocol::update_configuration::Setting::RetentionPeriodInSeconds as i32, - Some(0), - ) - .await - .unwrap(); let time_series = TestContext::generate_time_series_with_tag(false, None, Some("location")); ingest_time_series_and_flush_data( @@ -726,7 +726,10 @@ async fn test_can_vacuum_normal_table() { let files = std::fs::read_dir(&table_path).unwrap(); assert_eq!(files.count(), 2); - test_context.vacuum_table(NORMAL_TABLE_NAME).await.unwrap(); + test_context + .vacuum_table(NORMAL_TABLE_NAME, Some(0)) + .await + .unwrap(); // Only the _delta_log folder should remain. let files = std::fs::read_dir(&table_path).unwrap(); @@ -736,13 +739,6 @@ async fn test_can_vacuum_normal_table() { #[tokio::test] async fn test_can_vacuum_time_series_table() { let mut test_context = TestContext::new().await; - test_context - .update_configuration( - protocol::update_configuration::Setting::RetentionPeriodInSeconds as i32, - Some(0), - ) - .await - .unwrap(); let time_series = TestContext::generate_time_series_with_tag(false, None, Some("location")); ingest_time_series_and_flush_data( @@ -768,7 +764,7 @@ async fn test_can_vacuum_time_series_table() { assert_eq!(files.count(), 1); test_context - .vacuum_table(TIME_SERIES_TABLE_NAME) + .vacuum_table(TIME_SERIES_TABLE_NAME, Some(0)) .await .unwrap(); @@ -781,7 +777,7 @@ async fn test_can_vacuum_time_series_table() { async fn test_cannot_vacuum_missing_table() { let mut test_context = TestContext::new().await; - let result = test_context.vacuum_table(NORMAL_TABLE_NAME).await; + let result = test_context.vacuum_table(NORMAL_TABLE_NAME, None).await; assert!(result.is_err()); } From cfd123abeaf945b7e6983fa5a69cfce60d4d4af9 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 20:21:41 +0200 Subject: [PATCH 14/26] Add maybe_retention_period_in_seconds to Rust library --- .../src/operations/client.rs | 18 +++++++++++++++--- .../src/operations/data_folder.rs | 17 ++++++++++------- .../modelardb_embedded/src/operations/mod.rs | 10 ++++++++-- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/crates/modelardb_embedded/src/operations/client.rs b/crates/modelardb_embedded/src/operations/client.rs index 90d2f3808..9249fae73 100644 --- a/crates/modelardb_embedded/src/operations/client.rs +++ b/crates/modelardb_embedded/src/operations/client.rs @@ -358,10 +358,22 @@ impl Operations for Client { Ok(()) } - /// Vacuum the table with the name in `table_name`. If the table could not be vacuumed, + /// Vacuum the table with the name in `table_name` by deleting all files that are older than + /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the + /// default retention period of 7 days is used. If the table could not be vacuumed, /// [`ModelarDbEmbeddedError`] is returned. - async fn vacuum(&mut self, table_name: &str) -> Result<()> { - let ticket = Ticket::new(format!("VACUUM {table_name}")); + async fn vacuum( + &mut self, + table_name: &str, + maybe_retention_period_in_seconds: Option, + ) -> Result<()> { + let sql = if let Some(retention_period_in_seconds) = maybe_retention_period_in_seconds { + format!("VACUUM {table_name} RETAIN {retention_period_in_seconds}") + } else { + format!("VACUUM {table_name}") + }; + + let ticket = Ticket::new(sql); self.flight_client.do_get(ticket).await?; Ok(()) diff --git a/crates/modelardb_embedded/src/operations/data_folder.rs b/crates/modelardb_embedded/src/operations/data_folder.rs index c9fb7dca1..3260a3300 100644 --- a/crates/modelardb_embedded/src/operations/data_folder.rs +++ b/crates/modelardb_embedded/src/operations/data_folder.rs @@ -853,15 +853,18 @@ impl Operations for DataFolder { Ok(()) } - /// Vacuum the table with the name in `table_name`. If the table does not exist or the - /// table could not be vacuumed, [`ModelarDbEmbeddedError`] is returned. - async fn vacuum(&mut self, table_name: &str) -> Result<()> { + /// Vacuum the table with the name in `table_name` by deleting all files that are older than + /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the + /// default retention period of 7 days is used. If the table does not exist or the table could + /// not be vacuumed, [`ModelarDbEmbeddedError`] is returned. + async fn vacuum( + &mut self, + table_name: &str, + maybe_retention_period_in_seconds: Option, + ) -> Result<()> { if self.tables().await?.contains(&table_name.to_owned()) { - let retention_period_in_seconds = env::var("MODELARDBD_RETENTION_PERIOD_IN_SECONDS") - .map_or(60 * 60 * 24 * 7, |value| value.parse().unwrap()); - self.delta_lake - .vacuum_table(table_name, retention_period_in_seconds) + .vacuum_table(table_name, maybe_retention_period_in_seconds) .await .map_err(|error| error.into()) } else { diff --git a/crates/modelardb_embedded/src/operations/mod.rs b/crates/modelardb_embedded/src/operations/mod.rs index 650fd8545..5b805c8e6 100644 --- a/crates/modelardb_embedded/src/operations/mod.rs +++ b/crates/modelardb_embedded/src/operations/mod.rs @@ -108,8 +108,14 @@ pub trait Operations: Sync + Send { /// Drop the table with the name in `table_name`. async fn drop(&mut self, table_name: &str) -> Result<()>; - /// Vacuum the table with the name in `table_name`. - async fn vacuum(&mut self, table_name: &str) -> Result<()>; + /// Vacuum the table with the name in `table_name` by deleting all files that are older than + /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the + /// default retention period of 7 days is used. + async fn vacuum( + &mut self, + table_name: &str, + maybe_retention_period_in_seconds: Option, + ) -> Result<()>; } /// Use the time series table metadata in `table_name`, `schema`, `error_bounds`, and `generated_columns` From b022b5085d30ed1437479b559f52884bb942bb07 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 21:20:32 +0200 Subject: [PATCH 15/26] Use new interface in DataFolder tests --- .../src/operations/data_folder.rs | 30 ++++++------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/crates/modelardb_embedded/src/operations/data_folder.rs b/crates/modelardb_embedded/src/operations/data_folder.rs index 3260a3300..715286a77 100644 --- a/crates/modelardb_embedded/src/operations/data_folder.rs +++ b/crates/modelardb_embedded/src/operations/data_folder.rs @@ -17,7 +17,6 @@ use std::any::Any; use std::collections::HashMap; -use std::env; use std::fmt::{Debug, Formatter, Result as FmtResult}; use std::path::Path as StdPath; use std::pin::Pin; @@ -944,8 +943,6 @@ fn schemas_are_compatible(source_schema: &Schema, target_schema: &Schema) -> boo mod tests { use super::*; - use std::sync::{LazyLock, Mutex}; - use arrow::array::{Array, Float64Array, Int8Array, Int16Array, Int32Array, Int64Array}; use arrow::datatypes::{ArrowPrimitiveType, DataType, Field}; use arrow_flight::flight_service_client::FlightServiceClient; @@ -966,9 +963,6 @@ mod tests { const TIME_SERIES_TABLE_WITH_GENERATED_COLUMN_NAME: &str = "time_series_table_with_generated"; const INVALID_COLUMN_NAME: &str = "invalid_column"; - /// Lock used for env::set_var() as it is not guaranteed to be thread-safe. - static SET_VAR_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); - #[tokio::test] async fn test_create_normal_table() { let (_temp_dir, data_folder) = create_data_folder_with_normal_table().await; @@ -2569,12 +2563,6 @@ mod tests { #[tokio::test] async fn test_vacuum_normal_table() { - // env::set_var is safe to call in a single-threaded program. - unsafe { - let _mutex_guard = SET_VAR_LOCK.lock(); - env::set_var("MODELARDBD_RETENTION_PERIOD_IN_SECONDS", "0"); - } - let (temp_dir, mut data_folder) = create_data_folder_with_normal_table().await; data_folder @@ -2593,7 +2581,10 @@ mod tests { let files = std::fs::read_dir(&table_path).unwrap(); assert_eq!(files.count(), 2); - data_folder.vacuum(NORMAL_TABLE_NAME).await.unwrap(); + data_folder + .vacuum(NORMAL_TABLE_NAME, Some(0)) + .await + .unwrap(); // Only the _delta_log folder should remain. let files = std::fs::read_dir(&table_path).unwrap(); @@ -2602,12 +2593,6 @@ mod tests { #[tokio::test] async fn test_vacuum_time_series_table() { - // env::set_var is safe to call in a single-threaded program. - unsafe { - let _mutex_guard = SET_VAR_LOCK.lock(); - env::set_var("MODELARDBD_RETENTION_PERIOD_IN_SECONDS", "0"); - } - let (temp_dir, mut data_folder) = create_data_folder_with_time_series_table().await; data_folder @@ -2626,7 +2611,10 @@ mod tests { let files = std::fs::read_dir(&column_path).unwrap(); assert_eq!(files.count(), 1); - data_folder.vacuum(TIME_SERIES_TABLE_NAME).await.unwrap(); + data_folder + .vacuum(TIME_SERIES_TABLE_NAME, Some(0)) + .await + .unwrap(); // No files should remain in the column folder. let files = std::fs::read_dir(&column_path).unwrap(); @@ -2638,7 +2626,7 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let mut data_folder = DataFolder::open_local(temp_dir.path()).await.unwrap(); - let result = data_folder.vacuum(MISSING_TABLE_NAME).await; + let result = data_folder.vacuum(MISSING_TABLE_NAME, None).await; assert_eq!( result.unwrap_err().to_string(), From d5a38e513418efacd49fc6df91234223bbf42f1a Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 21:31:52 +0200 Subject: [PATCH 16/26] Add retention_period_in_seconds_ptr to vacuum() in C-API --- crates/modelardb_embedded/src/capi.rs | 35 +++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/crates/modelardb_embedded/src/capi.rs b/crates/modelardb_embedded/src/capi.rs index 604a24a8a..70782709a 100644 --- a/crates/modelardb_embedded/src/capi.rs +++ b/crates/modelardb_embedded/src/capi.rs @@ -933,15 +933,25 @@ unsafe fn drop( } /// Vacuums the table with the name in `table_name_ptr` in the [`DataFolder`] or [`Client`] in -/// `maybe_operations_ptr`. Assumes `maybe_operations_ptr` points to a [`DataFolder`] or [`Client`]; -/// and `table_name_ptr` points to a valid C string. +/// `maybe_operations_ptr` by deleting all files that are older than `retention_period_in_seconds_ptr` +/// seconds. Assumes `maybe_operations_ptr` points to a [`DataFolder`] or [`Client`]; +/// `table_name_ptr` points to a valid C string; and `retention_period_in_seconds_ptr` points to a +/// valid C string. #[unsafe(no_mangle)] pub unsafe extern "C" fn modelardb_embedded_vacuum( maybe_operations_ptr: *mut c_void, is_data_folder: bool, table_name_ptr: *const c_char, + retention_period_in_seconds_ptr: *const c_char, ) -> c_int { - let maybe_unit = unsafe { vacuum(maybe_operations_ptr, is_data_folder, table_name_ptr) }; + let maybe_unit = unsafe { + vacuum( + maybe_operations_ptr, + is_data_folder, + table_name_ptr, + retention_period_in_seconds_ptr, + ) + }; set_error_and_return_code(maybe_unit) } @@ -950,11 +960,26 @@ unsafe fn vacuum( maybe_operations_ptr: *mut c_void, is_data_folder: bool, table_name_ptr: *const c_char, + retention_period_in_seconds_ptr: *const c_char, ) -> Result<()> { let modelardb = unsafe { c_void_to_operations(maybe_operations_ptr, is_data_folder)? }; let table_name = unsafe { c_char_ptr_to_str(table_name_ptr)? }; - - TOKIO_RUNTIME.block_on(modelardb.vacuum(table_name)) + let maybe_retention_period_in_seconds_str = + unsafe { c_char_ptr_to_maybe_str(retention_period_in_seconds_ptr)? }; + + let maybe_retention_period_in_seconds = maybe_retention_period_in_seconds_str + .map(|retention_period_in_seconds_str| { + retention_period_in_seconds_str + .parse::() + .map_err(|error| { + ModelarDbEmbeddedError::InvalidArgument(format!( + "Retention period is not a valid u64: {error}" + )) + }) + }) + .transpose()?; + + TOKIO_RUNTIME.block_on(modelardb.vacuum(table_name, maybe_retention_period_in_seconds)) } /// Return a read-only [`*const c_char`] with a human-readable representation of the last error the From 0c0bd8eba74a99ebf19dd90fb6c2383dedec2861 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 21:46:23 +0200 Subject: [PATCH 17/26] Add retention_period_in_seconds to Python bindings --- .../bindings/python/modelardb/operations.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/modelardb_embedded/bindings/python/modelardb/operations.py b/crates/modelardb_embedded/bindings/python/modelardb/operations.py index 75a246591..b13c1d608 100644 --- a/crates/modelardb_embedded/bindings/python/modelardb/operations.py +++ b/crates/modelardb_embedded/bindings/python/modelardb/operations.py @@ -191,7 +191,8 @@ def __find_library(build: str) -> str: int modelardb_embedded_vacuum(void* maybe_operations_ptr, bool is_data_folder, - char* table_name_ptr); + char* table_name_ptr + char* retention_period_in_seconds_ptr); char* modelardb_embedded_error(); """ @@ -728,16 +729,26 @@ def drop(self, table_name: str): ) self.__check_return_code_and_raise_error(return_code) - def vacuum(self, table_name: str): + def vacuum(self, table_name: str, retention_period_in_seconds: None | int = None): """Vacuum the table with `table_name`. :param table_name: The name of the table to vacuum. :type table_name: str + :param retention_period_in_seconds: The retention period in seconds. Data older than the retention + period is deleted. If `None`, the default retention period of 7 days is used. + :type retention_period_in_seconds: int, optional :raises ValueError: If incorrect arguments are provided. """ table_name_ptr = ffi.new("char[]", bytes(table_name, "UTF-8")) + + if retention_period_in_seconds: + # Convert the retention period to a string to simplify the type conversion. + retention_period_in_seconds_ptr = ffi.new("char[]", bytes(str(retention_period_in_seconds), "UTF-8")) + else: + retention_period_in_seconds_ptr = ffi.NULL + return_code = self.__library.modelardb_embedded_vacuum( - self.__operations_ptr, self.__is_data_folder, table_name_ptr + self.__operations_ptr, self.__is_data_folder, table_name_ptr, retention_period_in_seconds_ptr ) self.__check_return_code_and_raise_error(return_code) From 389104cc36a673b84efb39e415b50fae6994eddf Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 21:57:02 +0200 Subject: [PATCH 18/26] Fix test_data_folder_vacuum test --- .../bindings/python/modelardb/operations.py | 4 ++-- .../bindings/python/tests/test_operations.py | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/modelardb_embedded/bindings/python/modelardb/operations.py b/crates/modelardb_embedded/bindings/python/modelardb/operations.py index b13c1d608..dbfdf2000 100644 --- a/crates/modelardb_embedded/bindings/python/modelardb/operations.py +++ b/crates/modelardb_embedded/bindings/python/modelardb/operations.py @@ -191,7 +191,7 @@ def __find_library(build: str) -> str: int modelardb_embedded_vacuum(void* maybe_operations_ptr, bool is_data_folder, - char* table_name_ptr + char* table_name_ptr, char* retention_period_in_seconds_ptr); char* modelardb_embedded_error(); @@ -741,7 +741,7 @@ def vacuum(self, table_name: str, retention_period_in_seconds: None | int = None """ table_name_ptr = ffi.new("char[]", bytes(table_name, "UTF-8")) - if retention_period_in_seconds: + if retention_period_in_seconds is not None: # Convert the retention period to a string to simplify the type conversion. retention_period_in_seconds_ptr = ffi.new("char[]", bytes(str(retention_period_in_seconds), "UTF-8")) else: diff --git a/crates/modelardb_embedded/bindings/python/tests/test_operations.py b/crates/modelardb_embedded/bindings/python/tests/test_operations.py index 1c96982b7..4e62c8cd4 100644 --- a/crates/modelardb_embedded/bindings/python/tests/test_operations.py +++ b/crates/modelardb_embedded/bindings/python/tests/test_operations.py @@ -439,8 +439,6 @@ def test_data_folder_drop_error(self): def test_data_folder_vacuum(self): with TemporaryDirectory() as temp_dir: - os.environ["MODELARDBD_RETENTION_PERIOD_IN_SECONDS"] = "0" - data_folder = Operations.open_local(temp_dir) create_tables_in_data_folder(data_folder) @@ -452,7 +450,7 @@ def test_data_folder_vacuum(self): file_count = len(os.listdir(folder_path)) self.assertEqual(file_count, 1) - data_folder.vacuum(TIME_SERIES_TABLE_NAME) + data_folder.vacuum(TIME_SERIES_TABLE_NAME, retention_period_in_seconds=0) # No files should remain in the column folder. file_count = len(os.listdir(folder_path)) From 53dc44f69288795dfc3f3d07f8c95c8049d8b280 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sat, 6 Sep 2025 22:34:54 +0200 Subject: [PATCH 19/26] Add check to ensure retention period is at most i64::MAX / 1000 --- crates/modelardb_storage/src/parser.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index d0c6a70ac..774d99c42 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -502,7 +502,8 @@ impl ModelarDbDialect { /// with the table names in the `channel` field and the optional retention period in the `payload` /// field. Note that [`Statement::NOTIFY`] is used since [`Statement`] does not have a `Vacuum` /// variant. A [`ParserError`] is returned if VACUUM is not the first word, the table names - /// cannot be extracted, or the retention period is not a valid positive integer. + /// cannot be extracted, or the retention period is not a valid positive integer that is at + /// most i64::MAX milliseconds. fn parse_vacuum(&self, parser: &mut Parser) -> StdResult { // VACUUM. parser.expect_keyword(Keyword::VACUUM)?; @@ -534,7 +535,16 @@ impl ModelarDbDialect { && word.keyword == Keyword::RETAIN { parser.expect_keyword(Keyword::RETAIN)?; - Some(self.parse_unsigned_literal_u64(parser)?) + let retention_period_in_seconds = self.parse_unsigned_literal_u64(parser)?; + + let max_retention_period_in_seconds = (i64::MAX / 1000) as u64; + if retention_period_in_seconds > max_retention_period_in_seconds { + return Err(ParserError::ParserError(format!( + "Retention period in seconds cannot be more than {max_retention_period_in_seconds} seconds." + ))); + } + + Some(retention_period_in_seconds) } else { None }; @@ -1798,8 +1808,8 @@ mod tests { } #[test] - fn test_tokenize_and_parse_vacuum_retain_with_u64_max_plus_one() { - let max_plus_one = u64::MAX as u128 + 1; + fn test_tokenize_and_parse_vacuum_retain_with_max_plus_one() { + let max_plus_one = (i64::MAX / 1000) + 1; assert!( tokenize_and_parse_sql_statement(&format!("VACUUM RETAIN {}", max_plus_one)).is_err() ); From bd32f1bd644c50094b0eaca76ead7f4ee297da1d Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sun, 7 Sep 2025 09:22:14 +0200 Subject: [PATCH 20/26] Fix minor consistency issue in tests and documentation --- crates/modelardb_storage/src/delta_lake.rs | 4 +++- crates/modelardb_storage/src/parser.rs | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index 0b915f70f..2194ca552 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -479,9 +479,11 @@ impl DeltaLake { let retention_period_in_seconds = maybe_retention_period_in_seconds.unwrap_or(60 * 60 * 24 * 7); + + let max_retention_period_in_seconds = i64::MAX / 1000; let retention_period = TimeDelta::new(retention_period_in_seconds as i64, 0).ok_or( ModelarDbStorageError::InvalidArgument(format!( - "Retention period of {retention_period_in_seconds} seconds is larger than i64::MAX milliseconds." + "Retention period in seconds cannot be more than {max_retention_period_in_seconds} seconds." )), )?; diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 774d99c42..620acbd20 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -1804,12 +1804,12 @@ mod tests { #[test] fn test_tokenize_and_parse_vacuum_retain_with_negative() { - assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN -5").is_err()); + assert!(tokenize_and_parse_sql_statement("VACUUM RETAIN -30").is_err()); } #[test] fn test_tokenize_and_parse_vacuum_retain_with_max_plus_one() { - let max_plus_one = (i64::MAX / 1000) + 1; + let max_plus_one = i64::MAX / 1000 + 1; assert!( tokenize_and_parse_sql_statement(&format!("VACUUM RETAIN {}", max_plus_one)).is_err() ); From 4b18f2287090b3349b73a2f05482f19dfc6bd666 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sun, 7 Sep 2025 09:40:06 +0200 Subject: [PATCH 21/26] Add constant for max retention period in seconds --- crates/modelardb_server/src/context.rs | 7 +++++-- crates/modelardb_storage/src/delta_lake.rs | 5 ++--- crates/modelardb_storage/src/parser.rs | 15 +++++++++------ crates/modelardb_types/src/types.rs | 4 ++++ 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/modelardb_server/src/context.rs b/crates/modelardb_server/src/context.rs index b34562fa5..4144b20d3 100644 --- a/crates/modelardb_server/src/context.rs +++ b/crates/modelardb_server/src/context.rs @@ -426,6 +426,7 @@ mod tests { use super::*; use modelardb_test::table::{self, NORMAL_TABLE_NAME, TIME_SERIES_TABLE_NAME}; + use modelardb_types::types::MAX_RETENTION_PERIOD_IN_SECONDS; use tempfile::TempDir; use crate::data_folders::DataFolder; @@ -834,10 +835,12 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let context = create_context_with_time_series_table(&temp_dir).await; - let retention_period_in_seconds = (i64::MAX / 1000 + 1) as u64; assert!( context - .vacuum_table(TIME_SERIES_TABLE_NAME, Some(retention_period_in_seconds)) + .vacuum_table( + TIME_SERIES_TABLE_NAME, + Some(MAX_RETENTION_PERIOD_IN_SECONDS + 1) + ) .await .is_err() ); diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index 2194ca552..53f6540be 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -37,7 +37,7 @@ use deltalake::{DeltaOps, DeltaTable, DeltaTableError}; use futures::{StreamExt, TryStreamExt}; use modelardb_types::flight::protocol; use modelardb_types::schemas::{COMPRESSED_SCHEMA, FIELD_COLUMN}; -use modelardb_types::types::TimeSeriesTableMetadata; +use modelardb_types::types::{MAX_RETENTION_PERIOD_IN_SECONDS, TimeSeriesTableMetadata}; use object_store::ObjectStore; use object_store::aws::AmazonS3Builder; use object_store::local::LocalFileSystem; @@ -480,10 +480,9 @@ impl DeltaLake { let retention_period_in_seconds = maybe_retention_period_in_seconds.unwrap_or(60 * 60 * 24 * 7); - let max_retention_period_in_seconds = i64::MAX / 1000; let retention_period = TimeDelta::new(retention_period_in_seconds as i64, 0).ok_or( ModelarDbStorageError::InvalidArgument(format!( - "Retention period in seconds cannot be more than {max_retention_period_in_seconds} seconds." + "Retention period in seconds cannot be more than {MAX_RETENTION_PERIOD_IN_SECONDS} seconds." )), )?; diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 620acbd20..8ed979e76 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -35,7 +35,8 @@ use datafusion::sql::TableReference; use datafusion::sql::planner::{ContextProvider, PlannerContext, SqlToRel}; use modelardb_types::functions::normalize_name; // Fully imported to not conflict. use modelardb_types::types::{ - ArrowTimestamp, ArrowValue, ErrorBound, GeneratedColumn, TimeSeriesTableMetadata, + ArrowTimestamp, ArrowValue, ErrorBound, GeneratedColumn, MAX_RETENTION_PERIOD_IN_SECONDS, + TimeSeriesTableMetadata, }; use sqlparser::ast::{ CascadeOption, ColumnDef, ColumnOption, ColumnOptionDef, CreateTable, DataType as SQLDataType, @@ -537,10 +538,9 @@ impl ModelarDbDialect { parser.expect_keyword(Keyword::RETAIN)?; let retention_period_in_seconds = self.parse_unsigned_literal_u64(parser)?; - let max_retention_period_in_seconds = (i64::MAX / 1000) as u64; - if retention_period_in_seconds > max_retention_period_in_seconds { + if retention_period_in_seconds > MAX_RETENTION_PERIOD_IN_SECONDS { return Err(ParserError::ParserError(format!( - "Retention period in seconds cannot be more than {max_retention_period_in_seconds} seconds." + "Retention period in seconds cannot be more than {MAX_RETENTION_PERIOD_IN_SECONDS} seconds." ))); } @@ -1809,9 +1809,12 @@ mod tests { #[test] fn test_tokenize_and_parse_vacuum_retain_with_max_plus_one() { - let max_plus_one = i64::MAX / 1000 + 1; assert!( - tokenize_and_parse_sql_statement(&format!("VACUUM RETAIN {}", max_plus_one)).is_err() + tokenize_and_parse_sql_statement(&format!( + "VACUUM RETAIN {}", + MAX_RETENTION_PERIOD_IN_SECONDS + 1 + )) + .is_err() ); } diff --git a/crates/modelardb_types/src/types.rs b/crates/modelardb_types/src/types.rs index 3a770bfeb..3301318bd 100644 --- a/crates/modelardb_types/src/types.rs +++ b/crates/modelardb_types/src/types.rs @@ -58,6 +58,10 @@ pub struct QueryCompressedSchema(pub Arc); #[derive(Clone)] pub struct GridSchema(pub Arc); +/// Maximum period in seconds that data can be retained in ModelarDB before it is deleted by a +/// VACUUM operation. The period is equal to the maximum value of an i64 in milliseconds, +pub const MAX_RETENTION_PERIOD_IN_SECONDS: u64 = (i64::MAX / 1000) as u64; + /// Types of tables supported by ModelarDB. pub enum Table { NormalTable(String, Schema), From bfdd6f5f7e45bf3ba1e334330c7d976f19542250 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sun, 7 Sep 2025 09:46:34 +0200 Subject: [PATCH 22/26] Use new constant in documentation --- crates/modelardb_server/src/context.rs | 5 +++-- crates/modelardb_storage/src/delta_lake.rs | 5 +++-- crates/modelardb_storage/src/parser.rs | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/modelardb_server/src/context.rs b/crates/modelardb_server/src/context.rs index 4144b20d3..a116298aa 100644 --- a/crates/modelardb_server/src/context.rs +++ b/crates/modelardb_server/src/context.rs @@ -332,8 +332,9 @@ impl Context { } /// Vacuum the table with `table_name` if it exists. If a retention period is not given, the - /// default retention period of 7 days is used. If the retention period is larger than i64::MAX - /// milliseconds, the table does not exist, or if it could not be vacuumed, + /// default retention period of 7 days is used. If the retention period is larger than + /// [`MAX_RETENTION_PERIOD_IN_SECONDS`](modelardb_types::types::MAX_RETENTION_PERIOD_IN_SECONDS) + /// seconds, the table does not exist, or if it could not be vacuumed, /// [`ModelarDbServerError`] is returned. pub async fn vacuum_table( &self, diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index 53f6540be..3c4c2fd39 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -468,8 +468,9 @@ impl DeltaLake { /// Vacuum the Delta Lake table with `table_name` by deleting all files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the - /// default retention period of 7 days is used. If the retention period is larger than i64::MAX - /// milliseconds or the files could not be deleted, a [`ModelarDbStorageError`] is returned. + /// default retention period of 7 days is used. If the retention period is larger than + /// [`MAX_RETENTION_PERIOD_IN_SECONDS`] seconds or the files could not be deleted, a + /// [`ModelarDbStorageError`] is returned. pub async fn vacuum_table( &self, table_name: &str, diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 8ed979e76..30b898228 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -504,7 +504,7 @@ impl ModelarDbDialect { /// field. Note that [`Statement::NOTIFY`] is used since [`Statement`] does not have a `Vacuum` /// variant. A [`ParserError`] is returned if VACUUM is not the first word, the table names /// cannot be extracted, or the retention period is not a valid positive integer that is at - /// most i64::MAX milliseconds. + /// most [`MAX_RETENTION_PERIOD_IN_SECONDS`] seconds. fn parse_vacuum(&self, parser: &mut Parser) -> StdResult { // VACUUM. parser.expect_keyword(Keyword::VACUUM)?; From 5ea3cb002a9bf75b86087d5765f4e799373eba62 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Sun, 7 Sep 2025 09:48:50 +0200 Subject: [PATCH 23/26] Fix punctuation error --- crates/modelardb_types/src/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/modelardb_types/src/types.rs b/crates/modelardb_types/src/types.rs index 3301318bd..a1875d013 100644 --- a/crates/modelardb_types/src/types.rs +++ b/crates/modelardb_types/src/types.rs @@ -59,7 +59,7 @@ pub struct QueryCompressedSchema(pub Arc); pub struct GridSchema(pub Arc); /// Maximum period in seconds that data can be retained in ModelarDB before it is deleted by a -/// VACUUM operation. The period is equal to the maximum value of an i64 in milliseconds, +/// VACUUM operation. The period is equal to the maximum value of an i64 in milliseconds. pub const MAX_RETENTION_PERIOD_IN_SECONDS: u64 = (i64::MAX / 1000) as u64; /// Types of tables supported by ModelarDB. From 0a92f6e39be99f0bbc8f133770067f4b8e0b49d4 Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Mon, 8 Sep 2025 09:36:57 +0200 Subject: [PATCH 24/26] Update based on comments from @skejserjensen --- .../bindings/python/modelardb/operations.py | 4 +++- crates/modelardb_embedded/src/capi.rs | 6 ++++-- crates/modelardb_embedded/src/operations/client.rs | 2 +- crates/modelardb_embedded/src/operations/data_folder.rs | 2 +- crates/modelardb_embedded/src/operations/mod.rs | 2 +- crates/modelardb_storage/src/delta_lake.rs | 2 +- crates/modelardb_types/src/types.rs | 2 ++ 7 files changed, 13 insertions(+), 7 deletions(-) diff --git a/crates/modelardb_embedded/bindings/python/modelardb/operations.py b/crates/modelardb_embedded/bindings/python/modelardb/operations.py index dbfdf2000..80e13f669 100644 --- a/crates/modelardb_embedded/bindings/python/modelardb/operations.py +++ b/crates/modelardb_embedded/bindings/python/modelardb/operations.py @@ -742,7 +742,9 @@ def vacuum(self, table_name: str, retention_period_in_seconds: None | int = None table_name_ptr = ffi.new("char[]", bytes(table_name, "UTF-8")) if retention_period_in_seconds is not None: - # Convert the retention period to a string to simplify the type conversion. + # Convert the retention period to a string to avoid issues with converting an int to a C type that uses + # an inconsistent amount of bits across platforms and then converting that to a 64-bit integer in Rust. + # The string is converted directly to an unsigned 64-bit integer in Rust. retention_period_in_seconds_ptr = ffi.new("char[]", bytes(str(retention_period_in_seconds), "UTF-8")) else: retention_period_in_seconds_ptr = ffi.NULL diff --git a/crates/modelardb_embedded/src/capi.rs b/crates/modelardb_embedded/src/capi.rs index 70782709a..64f865c1a 100644 --- a/crates/modelardb_embedded/src/capi.rs +++ b/crates/modelardb_embedded/src/capi.rs @@ -933,10 +933,12 @@ unsafe fn drop( } /// Vacuums the table with the name in `table_name_ptr` in the [`DataFolder`] or [`Client`] in -/// `maybe_operations_ptr` by deleting all files that are older than `retention_period_in_seconds_ptr` +/// `maybe_operations_ptr` by deleting stale files that are older than `retention_period_in_seconds_ptr` /// seconds. Assumes `maybe_operations_ptr` points to a [`DataFolder`] or [`Client`]; /// `table_name_ptr` points to a valid C string; and `retention_period_in_seconds_ptr` points to a -/// valid C string. +/// valid C string. A C string is used for the retention period to avoid issues with different +/// platforms using an inconsistent amount of bits for integer types. The string is converted +/// directly to an unsigned 64-bit integer in Rust. #[unsafe(no_mangle)] pub unsafe extern "C" fn modelardb_embedded_vacuum( maybe_operations_ptr: *mut c_void, diff --git a/crates/modelardb_embedded/src/operations/client.rs b/crates/modelardb_embedded/src/operations/client.rs index 9249fae73..40df35fdc 100644 --- a/crates/modelardb_embedded/src/operations/client.rs +++ b/crates/modelardb_embedded/src/operations/client.rs @@ -358,7 +358,7 @@ impl Operations for Client { Ok(()) } - /// Vacuum the table with the name in `table_name` by deleting all files that are older than + /// Vacuum the table with the name in `table_name` by deleting stale files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the /// default retention period of 7 days is used. If the table could not be vacuumed, /// [`ModelarDbEmbeddedError`] is returned. diff --git a/crates/modelardb_embedded/src/operations/data_folder.rs b/crates/modelardb_embedded/src/operations/data_folder.rs index 715286a77..0bb904b8f 100644 --- a/crates/modelardb_embedded/src/operations/data_folder.rs +++ b/crates/modelardb_embedded/src/operations/data_folder.rs @@ -852,7 +852,7 @@ impl Operations for DataFolder { Ok(()) } - /// Vacuum the table with the name in `table_name` by deleting all files that are older than + /// Vacuum the table with the name in `table_name` by deleting stale files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the /// default retention period of 7 days is used. If the table does not exist or the table could /// not be vacuumed, [`ModelarDbEmbeddedError`] is returned. diff --git a/crates/modelardb_embedded/src/operations/mod.rs b/crates/modelardb_embedded/src/operations/mod.rs index 5b805c8e6..e3185c86b 100644 --- a/crates/modelardb_embedded/src/operations/mod.rs +++ b/crates/modelardb_embedded/src/operations/mod.rs @@ -108,7 +108,7 @@ pub trait Operations: Sync + Send { /// Drop the table with the name in `table_name`. async fn drop(&mut self, table_name: &str) -> Result<()>; - /// Vacuum the table with the name in `table_name` by deleting all files that are older than + /// Vacuum the table with the name in `table_name` by deleting stale files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the /// default retention period of 7 days is used. async fn vacuum( diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index 3c4c2fd39..abe935590 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -466,7 +466,7 @@ impl DeltaLake { Ok(()) } - /// Vacuum the Delta Lake table with `table_name` by deleting all files that are older than + /// Vacuum the Delta Lake table with `table_name` by deleting stale files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the /// default retention period of 7 days is used. If the retention period is larger than /// [`MAX_RETENTION_PERIOD_IN_SECONDS`] seconds or the files could not be deleted, a diff --git a/crates/modelardb_types/src/types.rs b/crates/modelardb_types/src/types.rs index a1875d013..8af39a63e 100644 --- a/crates/modelardb_types/src/types.rs +++ b/crates/modelardb_types/src/types.rs @@ -60,6 +60,8 @@ pub struct GridSchema(pub Arc); /// Maximum period in seconds that data can be retained in ModelarDB before it is deleted by a /// VACUUM operation. The period is equal to the maximum value of an i64 in milliseconds. +/// The limitation is imposed by the use of `chrono::TimeDelta` when vacuuming data, which uses +/// an i64 internally to represent time in milliseconds. pub const MAX_RETENTION_PERIOD_IN_SECONDS: u64 = (i64::MAX / 1000) as u64; /// Types of tables supported by ModelarDB. From 1f366c1ef2eaca87c85fc926339ca540a57a88bd Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Mon, 8 Sep 2025 09:42:50 +0200 Subject: [PATCH 25/26] Add mention of max retention period to data folder and client methods --- crates/modelardb_embedded/src/operations/client.rs | 4 +++- crates/modelardb_embedded/src/operations/data_folder.rs | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/modelardb_embedded/src/operations/client.rs b/crates/modelardb_embedded/src/operations/client.rs index 40df35fdc..b12be79f0 100644 --- a/crates/modelardb_embedded/src/operations/client.rs +++ b/crates/modelardb_embedded/src/operations/client.rs @@ -360,7 +360,9 @@ impl Operations for Client { /// Vacuum the table with the name in `table_name` by deleting stale files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the - /// default retention period of 7 days is used. If the table could not be vacuumed, + /// default retention period of 7 days is used. If the table does not exist, the table could + /// not be vacuumed, or the retention period is larger than + /// [`MAX_RETENTION_PERIOD_IN_SECONDS`](modelardb_types::types::MAX_RETENTION_PERIOD_IN_SECONDS), /// [`ModelarDbEmbeddedError`] is returned. async fn vacuum( &mut self, diff --git a/crates/modelardb_embedded/src/operations/data_folder.rs b/crates/modelardb_embedded/src/operations/data_folder.rs index 0bb904b8f..f01c0c12f 100644 --- a/crates/modelardb_embedded/src/operations/data_folder.rs +++ b/crates/modelardb_embedded/src/operations/data_folder.rs @@ -854,8 +854,10 @@ impl Operations for DataFolder { /// Vacuum the table with the name in `table_name` by deleting stale files that are older than /// `maybe_retention_period_in_seconds` seconds. If a retention period is not given, the - /// default retention period of 7 days is used. If the table does not exist or the table could - /// not be vacuumed, [`ModelarDbEmbeddedError`] is returned. + /// default retention period of 7 days is used. If the table does not exist, the table could + /// not be vacuumed, or the retention period is larger than + /// [`MAX_RETENTION_PERIOD_IN_SECONDS`](modelardb_types::types::MAX_RETENTION_PERIOD_IN_SECONDS), + /// [`ModelarDbEmbeddedError`] is returned. async fn vacuum( &mut self, table_name: &str, From 627932dc0e072cd18e89f5c3222914830760826e Mon Sep 17 00:00:00 2001 From: CGodiksen <36046286+CGodiksen@users.noreply.github.com> Date: Tue, 9 Sep 2025 20:16:57 +0200 Subject: [PATCH 26/26] Update based on comments from @chrthomsen --- crates/modelardb_storage/src/delta_lake.rs | 2 +- crates/modelardb_storage/src/parser.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/modelardb_storage/src/delta_lake.rs b/crates/modelardb_storage/src/delta_lake.rs index abe935590..c80893e11 100644 --- a/crates/modelardb_storage/src/delta_lake.rs +++ b/crates/modelardb_storage/src/delta_lake.rs @@ -483,7 +483,7 @@ impl DeltaLake { let retention_period = TimeDelta::new(retention_period_in_seconds as i64, 0).ok_or( ModelarDbStorageError::InvalidArgument(format!( - "Retention period in seconds cannot be more than {MAX_RETENTION_PERIOD_IN_SECONDS} seconds." + "Retention period cannot be more than {MAX_RETENTION_PERIOD_IN_SECONDS} seconds." )), )?; diff --git a/crates/modelardb_storage/src/parser.rs b/crates/modelardb_storage/src/parser.rs index 30b898228..b35f43894 100644 --- a/crates/modelardb_storage/src/parser.rs +++ b/crates/modelardb_storage/src/parser.rs @@ -540,7 +540,7 @@ impl ModelarDbDialect { if retention_period_in_seconds > MAX_RETENTION_PERIOD_IN_SECONDS { return Err(ParserError::ParserError(format!( - "Retention period in seconds cannot be more than {MAX_RETENTION_PERIOD_IN_SECONDS} seconds." + "Retention period cannot be more than {MAX_RETENTION_PERIOD_IN_SECONDS} seconds." ))); }