-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support alter twcs compression options #4939
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
|
||
use std::collections::HashMap; | ||
use std::fmt; | ||
use std::str::FromStr; | ||
use std::time::Duration; | ||
|
||
use api::helper::ColumnDataTypeWrapper; | ||
|
@@ -25,6 +26,7 @@ use api::v1::region::{ | |
FlushRequest, InsertRequests, OpenRequest, TruncateRequest, | ||
}; | ||
use api::v1::{self, ChangeTableOption, Rows, SemanticType}; | ||
use common_base::readable_size::ReadableSize; | ||
pub use common_base::AffectedRows; | ||
use datatypes::data_type::ConcreteDataType; | ||
use serde::{Deserialize, Serialize}; | ||
|
@@ -36,7 +38,11 @@ use crate::metadata::{ | |
ColumnMetadata, InvalidRawRegionRequestSnafu, InvalidRegionOptionChangeRequestSnafu, | ||
InvalidRegionRequestSnafu, MetadataError, RegionMetadata, Result, | ||
}; | ||
use crate::mito_engine_options::TTL_KEY; | ||
use crate::mito_engine_options::{ | ||
TTL_KEY, TWCS_FALLBACK_TO_LOCAL, TWCS_MAX_ACTIVE_WINDOW_FILES, TWCS_MAX_ACTIVE_WINDOW_RUNS, | ||
TWCS_MAX_INACTIVE_WINDOW_FILES, TWCS_MAX_INACTIVE_WINDOW_RUNS, TWCS_MAX_OUTPUT_FILE_SIZE, | ||
TWCS_TIME_WINDOW, | ||
}; | ||
use crate::path_utils::region_dir; | ||
use crate::storage::{ColumnId, RegionId, ScanRequest}; | ||
|
||
|
@@ -661,23 +667,126 @@ impl From<v1::ChangeColumnType> for ChangeColumnType { | |
#[derive(Debug, Eq, PartialEq, Clone, Serialize, Deserialize)] | ||
pub enum ChangeOption { | ||
TTL(Duration), | ||
TwscMaxActiveWindowRuns(Option<usize>), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about changing these variants into one Twcs(String) The I think it looks much better and we can add new options in the future without adding new variant of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the draw back would be that we cannot validate the type of value at storage api layer - i.e user can pass max_active_window_runs='abcde' and this will error out at mito engine code path (where we handle the alter options). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can match the option to verify its validity, as the next comment explains. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah i see great call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took a try at implement this approach, I had a few concerns:
This is a draft of the working attempt https://github.com/GreptimeTeam/greptimedb/pull/4965/files if you like it i can push to this branch. |
||
TwscMaxActiveWindowFiles(Option<usize>), | ||
TwscMaxInactiveWindowRuns(Option<usize>), | ||
TwscMaxInactiveWindowFiles(Option<usize>), | ||
TwscMaxOutputFileSize(Option<ReadableSize>), | ||
TwscTimeWindow(Option<Duration>), | ||
TwscFallbackToLocal(Option<bool>), | ||
} | ||
|
||
impl ChangeOption { | ||
pub fn to_change_table_option(&self) -> ChangeTableOption { | ||
match self { | ||
ChangeOption::TTL(duration) => ChangeTableOption { | ||
key: "ttl".to_string(), | ||
value: humantime::format_duration(*duration).to_string(), | ||
}, | ||
ChangeOption::TwscMaxActiveWindowRuns(runs) => ChangeTableOption { | ||
key: TWCS_MAX_ACTIVE_WINDOW_RUNS.to_string(), | ||
value: runs.map(|s| s.to_string()).unwrap_or_default(), | ||
}, | ||
ChangeOption::TwscMaxActiveWindowFiles(files) => ChangeTableOption { | ||
key: TWCS_MAX_ACTIVE_WINDOW_FILES.to_string(), | ||
value: files.map(|s| s.to_string()).unwrap_or_default(), | ||
}, | ||
ChangeOption::TwscMaxInactiveWindowRuns(runs) => ChangeTableOption { | ||
key: TWCS_MAX_INACTIVE_WINDOW_RUNS.to_string(), | ||
value: runs.map(|s| s.to_string()).unwrap_or_default(), | ||
}, | ||
ChangeOption::TwscMaxInactiveWindowFiles(files) => ChangeTableOption { | ||
key: TWCS_MAX_INACTIVE_WINDOW_FILES.to_string(), | ||
value: files.map(|s| s.to_string()).unwrap_or_default(), | ||
}, | ||
ChangeOption::TwscMaxOutputFileSize(size) => ChangeTableOption { | ||
key: TWCS_MAX_OUTPUT_FILE_SIZE.to_string(), | ||
value: size.map(|s| s.to_string()).unwrap_or_default(), | ||
}, | ||
ChangeOption::TwscTimeWindow(window) => ChangeTableOption { | ||
key: TWCS_TIME_WINDOW.to_string(), | ||
value: window | ||
.map(|s| humantime::format_duration(s).to_string()) | ||
.unwrap_or_default(), | ||
}, | ||
ChangeOption::TwscFallbackToLocal(flag) => ChangeTableOption { | ||
key: TWCS_FALLBACK_TO_LOCAL.to_string(), | ||
value: flag.map(|s| s.to_string()).unwrap_or_default(), | ||
}, | ||
} | ||
} | ||
} | ||
|
||
impl TryFrom<&ChangeTableOption> for ChangeOption { | ||
type Error = MetadataError; | ||
|
||
fn try_from(value: &ChangeTableOption) -> std::result::Result<Self, Self::Error> { | ||
let ChangeTableOption { key, value } = value; | ||
if key == TTL_KEY { | ||
let ttl = if value.is_empty() { | ||
Duration::from_secs(0) | ||
|
||
// Inline helper for parsing Option<usize> | ||
let parse_optional_usize = |value: &str| { | ||
if value.is_empty() { | ||
Ok(None) | ||
} else { | ||
value | ||
.parse::<usize>() | ||
.map(Some) | ||
.map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build()) | ||
} | ||
}; | ||
|
||
// Inline helper for parsing Option<bool> | ||
let parse_optional_bool = |value: &str| { | ||
if value.is_empty() { | ||
Ok(None) | ||
} else { | ||
value | ||
.parse::<bool>() | ||
.map(Some) | ||
.map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build()) | ||
} | ||
}; | ||
|
||
// Inline helper for parsing Option<Duration> | ||
let parse_optional_duration = |value: &str| { | ||
if value.is_empty() { | ||
Ok(None) | ||
} else { | ||
humantime::parse_duration(value) | ||
.map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build())? | ||
}; | ||
Ok(Self::TTL(ttl)) | ||
} else { | ||
InvalidRegionOptionChangeRequestSnafu { key, value }.fail() | ||
.map(Some) | ||
.map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build()) | ||
} | ||
}; | ||
|
||
match key.as_str() { | ||
TTL_KEY => Ok(Self::TTL( | ||
parse_optional_duration(value)?.unwrap_or(Duration::from_secs(0)), | ||
)), | ||
TWCS_MAX_ACTIVE_WINDOW_RUNS => { | ||
Ok(Self::TwscMaxActiveWindowRuns(parse_optional_usize(value)?)) | ||
} | ||
TWCS_MAX_ACTIVE_WINDOW_FILES => { | ||
Ok(Self::TwscMaxActiveWindowFiles(parse_optional_usize(value)?)) | ||
} | ||
TWCS_MAX_INACTIVE_WINDOW_RUNS => Ok(Self::TwscMaxInactiveWindowRuns( | ||
parse_optional_usize(value)?, | ||
)), | ||
TWCS_MAX_INACTIVE_WINDOW_FILES => Ok(Self::TwscMaxInactiveWindowFiles( | ||
parse_optional_usize(value)?, | ||
)), | ||
TWCS_MAX_OUTPUT_FILE_SIZE => { | ||
let size = if value.is_empty() { | ||
None | ||
} else { | ||
Some(ReadableSize::from_str(value).map_err(|_| { | ||
InvalidRegionOptionChangeRequestSnafu { key, value }.build() | ||
})?) | ||
}; | ||
Ok(Self::TwscMaxOutputFileSize(size)) | ||
} | ||
TWCS_TIME_WINDOW => Ok(Self::TwscTimeWindow(parse_optional_duration(value)?)), | ||
TWCS_FALLBACK_TO_LOCAL => Ok(Self::TwscFallbackToLocal(parse_optional_bool(value)?)), | ||
_ => InvalidRegionOptionChangeRequestSnafu { key, value }.fail(), | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change all these variants into
Twcs(String)
, then we can refactor these match clauses into: