From 78211e4721bbac76c7048bf8c6e6e3bb42d84c01 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Wed, 24 Sep 2025 08:28:52 +0800 Subject: [PATCH 01/21] initial impl Signed-off-by: Ruihang Xia --- .../meta/src/ddl/alter_table/executor.rs | 84 +++++++- src/frontend/src/instance.rs | 7 + src/mito2/src/worker/handle_alter.rs | 3 + src/operator/src/statement.rs | 2 + src/operator/src/statement/comment.rs | 150 +++++++++++++ src/sql/src/parser.rs | 2 + src/sql/src/parsers.rs | 1 + src/sql/src/parsers/comment_parser.rs | 187 ++++++++++++++++ src/sql/src/statements.rs | 1 + src/sql/src/statements/comment.rs | 58 +++++ src/sql/src/statements/statement.rs | 5 + src/store-api/src/region_request.rs | 4 +- src/table/src/metadata.rs | 203 +++++++++++++++++- src/table/src/requests.rs | 1 + 14 files changed, 698 insertions(+), 10 deletions(-) create mode 100644 src/operator/src/statement/comment.rs create mode 100644 src/sql/src/parsers/comment_parser.rs create mode 100644 src/sql/src/statements/comment.rs diff --git a/src/common/meta/src/ddl/alter_table/executor.rs b/src/common/meta/src/ddl/alter_table/executor.rs index 99b6a1e012c2..480e56dbd741 100644 --- a/src/common/meta/src/ddl/alter_table/executor.rs +++ b/src/common/meta/src/ddl/alter_table/executor.rs @@ -25,9 +25,10 @@ use common_telemetry::{debug, info}; use futures::future; use snafu::{ResultExt, ensure}; use store_api::metadata::ColumnMetadata; +use store_api::region_request::SetRegionOption; use store_api::storage::{RegionId, TableId}; use table::metadata::{RawTableInfo, TableInfo}; -use table::requests::AlterKind; +use table::requests::{AlterKind, COMMENT_KEY}; use table::table_name::TableName; use crate::cache_invalidator::{CacheInvalidatorRef, Context}; @@ -297,11 +298,23 @@ fn build_new_table_info( } AlterKind::DropColumns { .. } | AlterKind::ModifyColumnTypes { .. } - | AlterKind::SetTableOptions { .. } | AlterKind::UnsetTableOptions { .. } | AlterKind::SetIndexes { .. } | AlterKind::UnsetIndexes { .. } | AlterKind::DropDefaults { .. } => {} + AlterKind::SetTableOptions { options } => { + for option in options { + if let SetRegionOption::Extra(key, value) = option { + if key == COMMENT_KEY { + new_info.desc = if value.is_empty() { + None + } else { + Some(value.clone()) + }; + } + } + } + } AlterKind::SetDefaults { .. } => {} } @@ -311,3 +324,70 @@ fn build_new_table_info( ); Ok(new_info) } + +#[cfg(test)] +mod tests { + use api::v1::alter_table_expr::Kind; + use api::v1::{AlterTableExpr, CreateTableExpr, Option as PbOption, SetTableOptions}; + use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME}; + + use super::*; + use crate::ddl::test_util::create_table::{ + TestCreateTableExprBuilder, build_raw_table_info_from_expr, + }; + + #[test] + fn test_build_new_table_info_updates_table_comment() { + let create_expr: CreateTableExpr = TestCreateTableExprBuilder::default() + .catalog_name(DEFAULT_CATALOG_NAME) + .schema_name(DEFAULT_SCHEMA_NAME) + .table_name("test") + .build() + .unwrap() + .into(); + let raw_table_info = build_raw_table_info_from_expr(&create_expr); + + let alter_expr = AlterTableExpr { + catalog_name: DEFAULT_CATALOG_NAME.to_string(), + schema_name: DEFAULT_SCHEMA_NAME.to_string(), + table_name: "test".to_string(), + kind: Some(Kind::SetTableOptions(SetTableOptions { + table_options: vec![PbOption { + key: COMMENT_KEY.to_string(), + value: "hello".to_string(), + }], + })), + }; + + let table_info = build_new_table_info(&raw_table_info, alter_expr).unwrap(); + assert_eq!(Some("hello".to_string()), table_info.desc); + } + + #[test] + fn test_build_new_table_info_clears_table_comment() { + let mut create_expr: CreateTableExpr = TestCreateTableExprBuilder::default() + .catalog_name(DEFAULT_CATALOG_NAME) + .schema_name(DEFAULT_SCHEMA_NAME) + .table_name("test") + .build() + .unwrap() + .into(); + create_expr.desc = "initial".to_string(); + let raw_table_info = build_raw_table_info_from_expr(&create_expr); + + let alter_expr = AlterTableExpr { + catalog_name: DEFAULT_CATALOG_NAME.to_string(), + schema_name: DEFAULT_SCHEMA_NAME.to_string(), + table_name: "test".to_string(), + kind: Some(Kind::SetTableOptions(SetTableOptions { + table_options: vec![PbOption { + key: COMMENT_KEY.to_string(), + value: String::new(), + }], + })), + }; + + let table_info = build_new_table_info(&raw_table_info, alter_expr).unwrap(); + assert_eq!(None, table_info.desc); + } +} diff --git a/src/frontend/src/instance.rs b/src/frontend/src/instance.rs index 7d4642f6c23f..406395ce7087 100644 --- a/src/frontend/src/instance.rs +++ b/src/frontend/src/instance.rs @@ -83,6 +83,7 @@ use snafu::prelude::*; use sql::ast::ObjectNamePartExt; use sql::dialect::Dialect; use sql::parser::{ParseOptions, ParserContext}; +use sql::statements::comment::CommentObject; use sql::statements::copy::{CopyDatabase, CopyTable}; use sql::statements::statement::Statement; use sql::statements::tql::Tql; @@ -904,6 +905,12 @@ pub fn check_permission( // show charset and show collation won't be checked Statement::ShowCharset(_) | Statement::ShowCollation(_) => {} + Statement::Comment(comment) => match &comment.object { + CommentObject::Table(table) => validate_param(table, query_ctx)?, + CommentObject::Column { table, .. } => validate_param(table, query_ctx)?, + CommentObject::Flow(flow) => validate_param(flow, query_ctx)?, + }, + Statement::Insert(insert) => { let name = insert.table_name().context(ParseSqlSnafu)?; validate_param(name, query_ctx)?; diff --git a/src/mito2/src/worker/handle_alter.rs b/src/mito2/src/worker/handle_alter.rs index aff97e09a139..080a66620654 100644 --- a/src/mito2/src/worker/handle_alter.rs +++ b/src/mito2/src/worker/handle_alter.rs @@ -186,6 +186,9 @@ impl RegionWorkerLoop { region.region_id, )?; } + SetRegionOption::Extra(_, _) => { + // Extra options are handled at metadata layer; no region side effect. + } } } region.version_control.alter_options(current_options); diff --git a/src/operator/src/statement.rs b/src/operator/src/statement.rs index 9434cc37e938..56fe495fc6e7 100644 --- a/src/operator/src/statement.rs +++ b/src/operator/src/statement.rs @@ -13,6 +13,7 @@ // limitations under the License. mod admin; +mod comment; mod copy_database; mod copy_query_to; mod copy_table_from; @@ -381,6 +382,7 @@ impl StatementExecutor { Statement::ShowCreateView(show) => self.show_create_view(show, query_ctx).await, Statement::SetVariables(set_var) => self.set_variables(set_var, query_ctx), Statement::ShowVariables(show_variable) => self.show_variable(show_variable, query_ctx), + Statement::Comment(stmt) => self.comment(stmt, query_ctx).await, Statement::ShowColumns(show_columns) => { self.show_columns(show_columns, query_ctx).await } diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs new file mode 100644 index 000000000000..292a58abf97b --- /dev/null +++ b/src/operator/src/statement/comment.rs @@ -0,0 +1,150 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use chrono::Utc; +use common_query::Output; +use session::context::QueryContextRef; +use snafu::{OptionExt, ResultExt}; +use sql::ast::{Ident, ObjectName, ObjectNamePartExt}; +use sql::statements::alter::{AlterTable, AlterTableOperation, KeyValueOption}; +use sql::statements::comment::{Comment, CommentObject}; +use table::requests::{COLUMN_COMMENT_PREFIX, COMMENT_KEY}; + +use crate::error::{FlowNotFoundSnafu, InvalidSqlSnafu, Result, TableMetadataManagerSnafu}; +use crate::statement::StatementExecutor; + +impl StatementExecutor { + pub async fn comment(&self, stmt: Comment, query_ctx: QueryContextRef) -> Result { + match stmt.object { + CommentObject::Table(table) => { + self.comment_on_table(table, stmt.comment, query_ctx).await + } + CommentObject::Column { table, column } => { + self.comment_on_column(table, column, stmt.comment, query_ctx) + .await + } + CommentObject::Flow(flow) => self.comment_on_flow(flow, stmt.comment, query_ctx).await, + } + } + + async fn comment_on_table( + &self, + table: ObjectName, + comment: Option, + query_ctx: QueryContextRef, + ) -> Result { + let value = comment.unwrap_or_default(); + let alter = AlterTable { + table_name: table, + alter_operation: AlterTableOperation::SetTableOptions { + options: vec![KeyValueOption { + key: COMMENT_KEY.to_string(), + value, + }], + }, + }; + self.alter_table(alter, query_ctx).await + } + + async fn comment_on_column( + &self, + table: ObjectName, + column: Ident, + comment: Option, + query_ctx: QueryContextRef, + ) -> Result { + // Ensure the column reference resolves so we can produce better error messages later + // by leveraging existing alter table validation. + let mut column_name = column.value.clone(); + if column_name.is_empty() { + column_name = column.to_string(); + } + + let key = format!("{}{}", COLUMN_COMMENT_PREFIX, column_name); + let value = comment.unwrap_or_default(); + + let alter = AlterTable { + table_name: table, + alter_operation: AlterTableOperation::SetTableOptions { + options: vec![KeyValueOption { key, value }], + }, + }; + self.alter_table(alter, query_ctx).await + } + + async fn comment_on_flow( + &self, + flow_name: ObjectName, + comment: Option, + query_ctx: QueryContextRef, + ) -> Result { + let (catalog_name, flow_name_str) = match &flow_name.0[..] { + [flow] => ( + query_ctx.current_catalog().to_string(), + flow.to_string_unquoted(), + ), + [catalog, flow] => (catalog.to_string_unquoted(), flow.to_string_unquoted()), + _ => { + return InvalidSqlSnafu { + err_msg: format!( + "expect flow name to be . or , actual: {flow_name}" + ), + } + .fail(); + } + }; + + let flow_name_val = self + .flow_metadata_manager + .flow_name_manager() + .get(&catalog_name, &flow_name_str) + .await + .context(TableMetadataManagerSnafu)? + .context(FlowNotFoundSnafu { + flow_name: &flow_name_str, + })?; + + let flow_id = flow_name_val.flow_id(); + let flow_info = self + .flow_metadata_manager + .flow_info_manager() + .get_raw(flow_id) + .await + .context(TableMetadataManagerSnafu)? + .context(FlowNotFoundSnafu { + flow_name: &flow_name_str, + })?; + + let mut new_flow_info = flow_info.get_inner_ref().clone(); + new_flow_info.comment = comment.unwrap_or_default(); + new_flow_info.updated_time = Utc::now(); + + let flow_routes = self + .flow_metadata_manager + .flow_route_manager() + .routes(flow_id) + .await + .context(TableMetadataManagerSnafu)? + .into_iter() + .map(|(key, value)| (key.partition_id(), value)) + .collect::>(); + + self.flow_metadata_manager + .update_flow_metadata(flow_id, &flow_info, &new_flow_info, flow_routes) + .await + .context(TableMetadataManagerSnafu)?; + + Ok(Output::new_with_affected_rows(0)) + } +} diff --git a/src/sql/src/parser.rs b/src/sql/src/parser.rs index 6e2a880348cd..310de35adb95 100644 --- a/src/sql/src/parser.rs +++ b/src/sql/src/parser.rs @@ -163,6 +163,8 @@ impl ParserContext<'_> { Keyword::TRUNCATE => self.parse_truncate(), + Keyword::COMMENT => self.parse_comment(), + Keyword::SET => self.parse_set_variables(), Keyword::ADMIN => self.parse_admin_command(), diff --git a/src/sql/src/parsers.rs b/src/sql/src/parsers.rs index e3c41c49b289..7d68d5d1ce0d 100644 --- a/src/sql/src/parsers.rs +++ b/src/sql/src/parsers.rs @@ -14,6 +14,7 @@ pub(crate) mod admin_parser; mod alter_parser; +pub(crate) mod comment_parser; pub(crate) mod copy_parser; pub(crate) mod create_parser; pub(crate) mod cursor_parser; diff --git a/src/sql/src/parsers/comment_parser.rs b/src/sql/src/parsers/comment_parser.rs new file mode 100644 index 000000000000..edf3def9213d --- /dev/null +++ b/src/sql/src/parsers/comment_parser.rs @@ -0,0 +1,187 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use snafu::{ResultExt, ensure}; +use sqlparser::ast::ObjectName; +use sqlparser::keywords::Keyword; +use sqlparser::tokenizer::Token; + +use crate::ast::{Ident, ObjectNamePart}; +use crate::error::{self, InvalidSqlSnafu, Result}; +use crate::parser::{FLOW, ParserContext}; +use crate::statements::comment::{Comment, CommentObject}; +use crate::statements::statement::Statement; + +impl ParserContext<'_> { + pub(crate) fn parse_comment(&mut self) -> Result { + let _ = self.parser.next_token(); // consume COMMENT + + if !self.parser.parse_keyword(Keyword::ON) { + return self.expected("ON", self.parser.peek_token()); + } + + let target_token = self.parser.next_token(); + let comment = match target_token.token { + Token::Word(word) if word.keyword == Keyword::TABLE => { + let raw_table = self.parse_object_name().context(error::UnexpectedSnafu { + expected: "a table name", + actual: self.peek_token_as_string(), + })?; + let table = Self::canonicalize_object_name(raw_table); + CommentObject::Table(table) + } + Token::Word(word) if word.keyword == Keyword::COLUMN => { + self.parse_column_comment_target()? + } + Token::Word(word) + if word.keyword == Keyword::NoKeyword && word.value.eq_ignore_ascii_case(FLOW) => + { + let raw_flow = self.parse_object_name().context(error::UnexpectedSnafu { + expected: "a flow name", + actual: self.peek_token_as_string(), + })?; + let flow = Self::canonicalize_object_name(raw_flow); + CommentObject::Flow(flow) + } + _ => return self.expected("TABLE, COLUMN or FLOW", target_token), + }; + + if !self.parser.parse_keyword(Keyword::IS) { + return self.expected("IS", self.parser.peek_token()); + } + + let comment_value = if self.parser.parse_keyword(Keyword::NULL) { + None + } else { + Some( + self.parser + .parse_literal_string() + .context(error::SyntaxSnafu)?, + ) + }; + + Ok(Statement::Comment(Comment { + object: comment, + comment: comment_value, + })) + } + + fn parse_column_comment_target(&mut self) -> Result { + let raw = self.parse_object_name().context(error::UnexpectedSnafu { + expected: "a column reference", + actual: self.peek_token_as_string(), + })?; + let canonical = Self::canonicalize_object_name(raw); + + let mut parts = canonical.0; + ensure!( + parts.len() >= 2, + InvalidSqlSnafu { + msg: "COMMENT ON COLUMN expects .".to_string(), + } + ); + + let column_part = parts.pop().unwrap(); + let ObjectNamePart::Identifier(column_ident) = column_part; + + let column = ParserContext::canonicalize_identifier(column_ident); + + let mut table_idents: Vec = Vec::with_capacity(parts.len()); + for part in parts { + match part { + ObjectNamePart::Identifier(ident) => { + table_idents.push(ident); + } + } + } + + ensure!( + !table_idents.is_empty(), + InvalidSqlSnafu { + msg: "Table name is required before column name".to_string(), + } + ); + + let table = ObjectName::from(table_idents); + + Ok(CommentObject::Column { table, column }) + } +} + +#[cfg(test)] +mod tests { + use std::assert_matches::assert_matches; + + use crate::dialect::GreptimeDbDialect; + use crate::parser::{ParseOptions, ParserContext}; + use crate::statements::comment::CommentObject; + use crate::statements::statement::Statement; + + fn parse(sql: &str) -> Statement { + let mut stmts = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(stmts.len(), 1); + stmts.pop().unwrap() + } + + #[test] + fn test_parse_comment_on_table() { + let stmt = parse("COMMENT ON TABLE mytable IS 'test';"); + match stmt { + Statement::Comment(comment) => { + assert_matches!(comment.object, CommentObject::Table(ref name) if name.to_string() == "mytable"); + assert_eq!(comment.comment.as_deref(), Some("test")); + } + _ => panic!("expected comment statement"), + } + + let stmt = parse("COMMENT ON TABLE mytable IS NULL;"); + match stmt { + Statement::Comment(comment) => { + assert_matches!(comment.object, CommentObject::Table(ref name) if name.to_string() == "mytable"); + assert!(comment.comment.is_none()); + } + _ => panic!("expected comment statement"), + } + } + + #[test] + fn test_parse_comment_on_column() { + let stmt = parse("COMMENT ON COLUMN my_schema.my_table.my_col IS 'desc';"); + match stmt { + Statement::Comment(comment) => match comment.object { + CommentObject::Column { table, column } => { + assert_eq!(table.to_string(), "my_schema.my_table"); + assert_eq!(column.value, "my_col"); + assert_eq!(comment.comment.as_deref(), Some("desc")); + } + _ => panic!("expected column comment"), + }, + _ => panic!("expected comment statement"), + } + } + + #[test] + fn test_parse_comment_on_flow() { + let stmt = parse("COMMENT ON FLOW my_flow IS 'desc';"); + match stmt { + Statement::Comment(comment) => { + assert_matches!(comment.object, CommentObject::Flow(ref name) if name.to_string() == "my_flow"); + assert_eq!(comment.comment.as_deref(), Some("desc")); + } + _ => panic!("expected comment statement"), + } + } +} diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 488fadfae6dc..a9f793ecb5b7 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -14,6 +14,7 @@ pub mod admin; pub mod alter; +pub mod comment; pub mod copy; pub mod create; pub mod cursor; diff --git a/src/sql/src/statements/comment.rs b/src/sql/src/statements/comment.rs new file mode 100644 index 000000000000..ab8bae56a26a --- /dev/null +++ b/src/sql/src/statements/comment.rs @@ -0,0 +1,58 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::fmt::{self, Display, Formatter}; + +use serde::Serialize; +use sqlparser_derive::{Visit, VisitMut}; + +use crate::ast::{Ident, ObjectName}; + +#[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] +pub struct Comment { + pub object: CommentObject, + pub comment: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] +pub enum CommentObject { + Table(ObjectName), + Column { table: ObjectName, column: Ident }, + Flow(ObjectName), +} + +impl Display for Comment { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "COMMENT ON {} IS ", self.object)?; + match &self.comment { + Some(comment) => { + let escaped = comment.replace('\'', "''"); + write!(f, "'{}'", escaped) + } + None => f.write_str("NULL"), + } + } +} + +impl Display for CommentObject { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + match self { + CommentObject::Table(name) => write!(f, "TABLE {}", name), + CommentObject::Column { table, column } => { + write!(f, "COLUMN {}.{}", table, column) + } + CommentObject::Flow(name) => write!(f, "FLOW {}", name), + } + } +} diff --git a/src/sql/src/statements/statement.rs b/src/sql/src/statements/statement.rs index b1f7e5d7129a..664b3342d09a 100644 --- a/src/sql/src/statements/statement.rs +++ b/src/sql/src/statements/statement.rs @@ -22,6 +22,7 @@ use sqlparser_derive::{Visit, VisitMut}; use crate::error::{ConvertToDfStatementSnafu, Error}; use crate::statements::admin::Admin; use crate::statements::alter::{AlterDatabase, AlterTable}; +use crate::statements::comment::Comment; use crate::statements::copy::Copy; use crate::statements::create::{ CreateDatabase, CreateExternalTable, CreateFlow, CreateTable, CreateTableLike, CreateView, @@ -135,6 +136,8 @@ pub enum Statement { SetVariables(SetVariables), // SHOW VARIABLES ShowVariables(ShowVariables), + // COMMENT ON + Comment(Comment), // USE Use(String), // Admin statement(extension) @@ -200,6 +203,7 @@ impl Statement { | Statement::Copy(_) | Statement::TruncateTable(_) | Statement::SetVariables(_) + | Statement::Comment(_) | Statement::Use(_) | Statement::DeclareCursor(_) | Statement::CloseCursor(_) @@ -261,6 +265,7 @@ impl Display for Statement { Statement::TruncateTable(s) => s.fmt(f), Statement::SetVariables(s) => s.fmt(f), Statement::ShowVariables(s) => s.fmt(f), + Statement::Comment(s) => s.fmt(f), Statement::ShowCharset(kind) => { write!(f, "SHOW CHARSET {kind}") } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index c44a225d6cc3..d71b67255383 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -1246,6 +1246,8 @@ pub enum SetRegionOption { Ttl(Option), // Modifying TwscOptions with values as (option name, new value). Twsc(String, String), + /// Extra table options forwarded to metadata only. + Extra(String, String), } impl TryFrom<&PbOption> for SetRegionOption { @@ -1263,7 +1265,7 @@ impl TryFrom<&PbOption> for SetRegionOption { TWCS_TRIGGER_FILE_NUM | TWCS_MAX_OUTPUT_FILE_SIZE | TWCS_TIME_WINDOW => { Ok(Self::Twsc(key.to_string(), value.to_string())) } - _ => InvalidSetRegionOptionRequestSnafu { key, value }.fail(), + _ => Ok(Self::Extra(key.to_string(), value.to_string())), } } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 3704bc889f61..64a721b8150d 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -35,8 +35,8 @@ use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, Re use crate::error::{self, Result}; use crate::requests::{ - AddColumnRequest, AlterKind, ModifyColumnTypeRequest, SetDefaultRequest, SetIndexOption, - TableOptions, UnsetIndexOption, + AddColumnRequest, AlterKind, COLUMN_COMMENT_PREFIX, COMMENT_KEY, ModifyColumnTypeRequest, + SetDefaultRequest, SetIndexOption, TableOptions, UnsetIndexOption, }; use crate::table_reference::TableReference; @@ -241,8 +241,8 @@ impl TableMeta { } // No need to rebuild table meta when renaming tables. AlterKind::RenameTable { .. } => Ok(self.new_meta_builder()), - AlterKind::SetTableOptions { options } => self.set_table_options(options), - AlterKind::UnsetTableOptions { keys } => self.unset_table_options(keys), + AlterKind::SetTableOptions { options } => self.set_table_options(table_name, options), + AlterKind::UnsetTableOptions { keys } => self.unset_table_options(table_name, keys), AlterKind::SetIndexes { options } => self.set_indexes(table_name, options), AlterKind::UnsetIndexes { options } => self.unset_indexes(table_name, options), AlterKind::DropDefaults { names } => self.drop_defaults(table_name, names), @@ -251,8 +251,14 @@ impl TableMeta { } /// Creates a [TableMetaBuilder] with modified table options. - fn set_table_options(&self, requests: &[SetRegionOption]) -> Result { + fn set_table_options( + &self, + table_name: &str, + requests: &[SetRegionOption], + ) -> Result { let mut new_options = self.options.clone(); + let table_schema = &self.schema; + let mut column_comment_updates = HashMap::new(); for request in requests { match request { @@ -274,17 +280,96 @@ impl TableMeta { new_options.extra_options.remove(key.as_str()); } } + SetRegionOption::Extra(key, value) => { + if key.starts_with(COLUMN_COMMENT_PREFIX) { + let column_name = key[COLUMN_COMMENT_PREFIX.len()..].to_string(); + let update = if value.is_empty() { + None + } else { + Some(value.to_string()) + }; + column_comment_updates.insert(column_name, update); + } else if value.is_empty() { + new_options.extra_options.remove(key); + } else { + new_options + .extra_options + .insert(key.to_string(), value.to_string()); + } + } } } + + if let Some(comment) = new_options.extra_options.get(COMMENT_KEY) + && comment.is_empty() + { + new_options.extra_options.remove(COMMENT_KEY); + } + let mut builder = self.new_meta_builder(); builder.options(new_options); + if !column_comment_updates.is_empty() { + let mut columns = Vec::with_capacity(table_schema.column_schemas().len()); + let mut remaining = column_comment_updates; + + for mut column in table_schema.column_schemas().iter().cloned() { + if let Some(update) = remaining.remove(&column.name) { + match update { + Some(comment) => { + column + .mut_metadata() + .insert(datatypes::schema::COMMENT_KEY.to_string(), comment); + } + None => { + column.mut_metadata().remove(datatypes::schema::COMMENT_KEY); + } + } + } + columns.push(column); + } + + if let Some((missing, _)) = remaining.into_iter().next() { + return error::ColumnNotExistsSnafu { + column_name: &missing, + table_name, + } + .fail(); + } + + let mut schema_builder = SchemaBuilder::try_from_columns(columns) + .with_context(|_| error::SchemaBuildSnafu { + msg: format!( + "Failed to rebuild schema for table {table_name} when updating column comments", + ), + })? + .version(table_schema.version() + 1); + + for (k, v) in table_schema.metadata().iter() { + schema_builder = schema_builder.add_metadata(k, v); + } + + let new_schema = schema_builder + .build() + .with_context(|_| error::SchemaBuildSnafu { + msg: format!("Table {table_name} cannot update column comments",), + })?; + + let _ = builder + .schema(Arc::new(new_schema)) + .primary_key_indices(self.primary_key_indices.clone()); + } + Ok(builder) } - fn unset_table_options(&self, requests: &[UnsetRegionOption]) -> Result { + fn unset_table_options( + &self, + table_name: &str, + requests: &[UnsetRegionOption], + ) -> Result { let requests = requests.iter().map(Into::into).collect::>(); - self.set_table_options(&requests) + self.set_table_options(table_name, &requests) } fn set_indexes( @@ -1898,6 +1983,110 @@ mod tests { assert_matches!(err, Error::RemovePartitionColumn { .. }); } + #[test] + fn test_set_column_comment_via_table_options() { + let schema = Arc::new(new_test_schema()); + let meta = TableMetaBuilder::empty() + .schema(schema) + .primary_key_indices(vec![0]) + .engine("engine") + .next_column_id(3) + .build() + .unwrap(); + + let alter_kind = AlterKind::SetTableOptions { + options: vec![SetRegionOption::Extra( + format!("{}{}", COLUMN_COMMENT_PREFIX, "col1"), + "first column".to_string(), + )], + }; + + let new_meta = meta + .builder_with_alter_kind("my_table", &alter_kind) + .unwrap() + .build() + .unwrap(); + + let col_schema = new_meta.schema.column_schema_by_name("col1").unwrap(); + assert_eq!( + Some(&"first column".to_string()), + col_schema.metadata().get(datatypes::schema::COMMENT_KEY) + ); + assert_eq!(meta.schema.version() + 1, new_meta.schema.version()); + } + + #[test] + fn test_remove_column_comment_via_table_options() { + let schema = Arc::new(new_test_schema()); + let meta = TableMetaBuilder::empty() + .schema(schema) + .primary_key_indices(vec![0]) + .engine("engine") + .next_column_id(3) + .build() + .unwrap(); + + let add_comment = AlterKind::SetTableOptions { + options: vec![SetRegionOption::Extra( + format!("{}{}", COLUMN_COMMENT_PREFIX, "col1"), + "first column".to_string(), + )], + }; + let meta_with_comment = meta + .builder_with_alter_kind("my_table", &add_comment) + .unwrap() + .build() + .unwrap(); + + let remove_comment = AlterKind::SetTableOptions { + options: vec![SetRegionOption::Extra( + format!("{}{}", COLUMN_COMMENT_PREFIX, "col1"), + String::new(), + )], + }; + let meta_without_comment = meta_with_comment + .builder_with_alter_kind("my_table", &remove_comment) + .unwrap() + .build() + .unwrap(); + + let col_schema = meta_without_comment + .schema + .column_schema_by_name("col1") + .unwrap(); + assert!( + col_schema + .metadata() + .get(datatypes::schema::COMMENT_KEY) + .is_none() + ); + } + + #[test] + fn test_set_column_comment_unknown_column() { + let schema = Arc::new(new_test_schema()); + let meta = TableMetaBuilder::empty() + .schema(schema) + .primary_key_indices(vec![0]) + .engine("engine") + .next_column_id(3) + .build() + .unwrap(); + + let alter_kind = AlterKind::SetTableOptions { + options: vec![SetRegionOption::Extra( + format!("{}{}", COLUMN_COMMENT_PREFIX, "unknown"), + "comment".to_string(), + )], + }; + + let err = meta + .builder_with_alter_kind("my_table", &alter_kind) + .err() + .unwrap(); + assert_eq!(StatusCode::TableColumnNotFound, err.status_code()); + } + #[test] fn test_change_key_column_data_type() { let schema = Arc::new(new_test_schema()); diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 8f64f0b9fc8f..ad13e97b7df2 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -135,6 +135,7 @@ pub const WRITE_BUFFER_SIZE_KEY: &str = "write_buffer_size"; pub const TTL_KEY: &str = store_api::mito_engine_options::TTL_KEY; pub const STORAGE_KEY: &str = "storage"; pub const COMMENT_KEY: &str = "comment"; +pub const COLUMN_COMMENT_PREFIX: &str = "column.comment."; pub const AUTO_CREATE_TABLE_KEY: &str = "auto_create_table"; pub const SKIP_WAL_KEY: &str = store_api::mito_engine_options::SKIP_WAL_KEY; From cd65bab4de2bda967212bf1ead5f703916f945eb Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Wed, 24 Sep 2025 08:51:45 +0800 Subject: [PATCH 02/21] simplify impl Signed-off-by: Ruihang Xia --- .../meta/src/ddl/alter_table/executor.rs | 88 +------- src/mito2/src/worker/handle_alter.rs | 3 - src/operator/src/statement/comment.rs | 167 +++++++++++--- src/store-api/src/region_request.rs | 4 +- src/table/src/metadata.rs | 203 +----------------- src/table/src/requests.rs | 1 - 6 files changed, 151 insertions(+), 315 deletions(-) diff --git a/src/common/meta/src/ddl/alter_table/executor.rs b/src/common/meta/src/ddl/alter_table/executor.rs index 480e56dbd741..6e8962fbd05f 100644 --- a/src/common/meta/src/ddl/alter_table/executor.rs +++ b/src/common/meta/src/ddl/alter_table/executor.rs @@ -25,10 +25,9 @@ use common_telemetry::{debug, info}; use futures::future; use snafu::{ResultExt, ensure}; use store_api::metadata::ColumnMetadata; -use store_api::region_request::SetRegionOption; use store_api::storage::{RegionId, TableId}; use table::metadata::{RawTableInfo, TableInfo}; -use table::requests::{AlterKind, COMMENT_KEY}; +use table::requests::AlterKind; use table::table_name::TableName; use crate::cache_invalidator::{CacheInvalidatorRef, Context}; @@ -298,24 +297,12 @@ fn build_new_table_info( } AlterKind::DropColumns { .. } | AlterKind::ModifyColumnTypes { .. } + | AlterKind::SetTableOptions { .. } | AlterKind::UnsetTableOptions { .. } | AlterKind::SetIndexes { .. } | AlterKind::UnsetIndexes { .. } - | AlterKind::DropDefaults { .. } => {} - AlterKind::SetTableOptions { options } => { - for option in options { - if let SetRegionOption::Extra(key, value) = option { - if key == COMMENT_KEY { - new_info.desc = if value.is_empty() { - None - } else { - Some(value.clone()) - }; - } - } - } - } - AlterKind::SetDefaults { .. } => {} + | AlterKind::DropDefaults { .. } + | AlterKind::SetDefaults { .. } => {} } info!( @@ -324,70 +311,3 @@ fn build_new_table_info( ); Ok(new_info) } - -#[cfg(test)] -mod tests { - use api::v1::alter_table_expr::Kind; - use api::v1::{AlterTableExpr, CreateTableExpr, Option as PbOption, SetTableOptions}; - use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME}; - - use super::*; - use crate::ddl::test_util::create_table::{ - TestCreateTableExprBuilder, build_raw_table_info_from_expr, - }; - - #[test] - fn test_build_new_table_info_updates_table_comment() { - let create_expr: CreateTableExpr = TestCreateTableExprBuilder::default() - .catalog_name(DEFAULT_CATALOG_NAME) - .schema_name(DEFAULT_SCHEMA_NAME) - .table_name("test") - .build() - .unwrap() - .into(); - let raw_table_info = build_raw_table_info_from_expr(&create_expr); - - let alter_expr = AlterTableExpr { - catalog_name: DEFAULT_CATALOG_NAME.to_string(), - schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: "test".to_string(), - kind: Some(Kind::SetTableOptions(SetTableOptions { - table_options: vec![PbOption { - key: COMMENT_KEY.to_string(), - value: "hello".to_string(), - }], - })), - }; - - let table_info = build_new_table_info(&raw_table_info, alter_expr).unwrap(); - assert_eq!(Some("hello".to_string()), table_info.desc); - } - - #[test] - fn test_build_new_table_info_clears_table_comment() { - let mut create_expr: CreateTableExpr = TestCreateTableExprBuilder::default() - .catalog_name(DEFAULT_CATALOG_NAME) - .schema_name(DEFAULT_SCHEMA_NAME) - .table_name("test") - .build() - .unwrap() - .into(); - create_expr.desc = "initial".to_string(); - let raw_table_info = build_raw_table_info_from_expr(&create_expr); - - let alter_expr = AlterTableExpr { - catalog_name: DEFAULT_CATALOG_NAME.to_string(), - schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: "test".to_string(), - kind: Some(Kind::SetTableOptions(SetTableOptions { - table_options: vec![PbOption { - key: COMMENT_KEY.to_string(), - value: String::new(), - }], - })), - }; - - let table_info = build_new_table_info(&raw_table_info, alter_expr).unwrap(); - assert_eq!(None, table_info.desc); - } -} diff --git a/src/mito2/src/worker/handle_alter.rs b/src/mito2/src/worker/handle_alter.rs index 080a66620654..aff97e09a139 100644 --- a/src/mito2/src/worker/handle_alter.rs +++ b/src/mito2/src/worker/handle_alter.rs @@ -186,9 +186,6 @@ impl RegionWorkerLoop { region.region_id, )?; } - SetRegionOption::Extra(_, _) => { - // Extra options are handled at metadata layer; no region side effect. - } } } region.version_control.alter_options(current_options); diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index 292a58abf97b..b8b415744f03 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -13,17 +13,37 @@ // limitations under the License. use chrono::Utc; +use common_catalog::format_full_table_name; +use common_error::ext::BoxedError; +use common_meta::cache_invalidator::Context; +use common_meta::instruction::CacheIdent; +use common_meta::key::DeserializedValueWithBytes; +use common_meta::key::table_info::TableInfoValue; use common_query::Output; use session::context::QueryContextRef; +use session::table_name::table_idents_to_full_name; use snafu::{OptionExt, ResultExt}; use sql::ast::{Ident, ObjectName, ObjectNamePartExt}; -use sql::statements::alter::{AlterTable, AlterTableOperation, KeyValueOption}; use sql::statements::comment::{Comment, CommentObject}; -use table::requests::{COLUMN_COMMENT_PREFIX, COMMENT_KEY}; +use table::metadata::TableId; +use table::requests::COMMENT_KEY; +use table::table_name::TableName; -use crate::error::{FlowNotFoundSnafu, InvalidSqlSnafu, Result, TableMetadataManagerSnafu}; +use crate::error::{ + CatalogSnafu, ColumnNotFoundSnafu, ExternalSnafu, FlowNotFoundSnafu, InvalidSqlSnafu, + InvalidateTableCacheSnafu, Result, TableMetadataManagerSnafu, TableNotFoundSnafu, +}; use crate::statement::StatementExecutor; +struct ResolvedTable { + catalog: String, + schema: String, + table: String, + table_id: TableId, + table_name: TableName, + table_info: DeserializedValueWithBytes, +} + impl StatementExecutor { pub async fn comment(&self, stmt: Comment, query_ctx: QueryContextRef) -> Result { match stmt.object { @@ -44,17 +64,25 @@ impl StatementExecutor { comment: Option, query_ctx: QueryContextRef, ) -> Result { - let value = comment.unwrap_or_default(); - let alter = AlterTable { - table_name: table, - alter_operation: AlterTableOperation::SetTableOptions { - options: vec![KeyValueOption { - key: COMMENT_KEY.to_string(), - value, - }], - }, - }; - self.alter_table(alter, query_ctx).await + let ResolvedTable { + table_id, + table_name, + table_info, + .. + } = self.resolve_table_for_comment(&table, &query_ctx).await?; + + let mut new_table_info = table_info.table_info.clone(); + new_table_info.desc = comment; + + self.table_metadata_manager + .update_table_info(&table_info, None, new_table_info) + .await + .context(TableMetadataManagerSnafu)?; + + self.invalidate_table_cache_by_name(table_id, table_name) + .await?; + + Ok(Output::new_with_affected_rows(0)) } async fn comment_on_column( @@ -64,23 +92,49 @@ impl StatementExecutor { comment: Option, query_ctx: QueryContextRef, ) -> Result { - // Ensure the column reference resolves so we can produce better error messages later - // by leveraging existing alter table validation. - let mut column_name = column.value.clone(); - if column_name.is_empty() { - column_name = column.to_string(); + let ResolvedTable { + catalog, + schema, + table, + table_id, + table_name, + table_info, + } = self.resolve_table_for_comment(&table, &query_ctx).await?; + + let full_table_name = format_full_table_name(&catalog, &schema, &table); + let mut new_table_info = table_info.table_info.clone(); + + let column_name = column.value.clone(); + let column_schema = new_table_info + .meta + .schema + .column_schemas + .iter_mut() + .find(|col| col.name == column_name) + .with_context(|| ColumnNotFoundSnafu { + msg: format!("{} column `{}`", full_table_name, column_name), + })?; + + match comment { + Some(value) => { + column_schema + .mut_metadata() + .insert(COMMENT_KEY.to_string(), value); + } + None => { + column_schema.mut_metadata().remove(COMMENT_KEY); + } } - let key = format!("{}{}", COLUMN_COMMENT_PREFIX, column_name); - let value = comment.unwrap_or_default(); + self.table_metadata_manager + .update_table_info(&table_info, None, new_table_info) + .await + .context(TableMetadataManagerSnafu)?; - let alter = AlterTable { - table_name: table, - alter_operation: AlterTableOperation::SetTableOptions { - options: vec![KeyValueOption { key, value }], - }, - }; - self.alter_table(alter, query_ctx).await + self.invalidate_table_cache_by_name(table_id, table_name) + .await?; + + Ok(Output::new_with_affected_rows(0)) } async fn comment_on_flow( @@ -147,4 +201,61 @@ impl StatementExecutor { Ok(Output::new_with_affected_rows(0)) } + + async fn resolve_table_for_comment( + &self, + table: &ObjectName, + query_ctx: &QueryContextRef, + ) -> Result { + let (catalog_name, schema_name, table_name) = table_idents_to_full_name(table, query_ctx) + .map_err(BoxedError::new) + .context(ExternalSnafu)?; + + let full_table_name = format_full_table_name(&catalog_name, &schema_name, &table_name); + + let table_ref = self + .catalog_manager + .table(&catalog_name, &schema_name, &table_name, Some(query_ctx)) + .await + .context(CatalogSnafu)? + .with_context(|| TableNotFoundSnafu { + table_name: full_table_name.clone(), + })?; + + let table_id = table_ref.table_info().table_id(); + let table_info = self + .table_metadata_manager + .table_info_manager() + .get(table_id) + .await + .context(TableMetadataManagerSnafu)? + .context(TableNotFoundSnafu { + table_name: full_table_name, + })?; + + Ok(ResolvedTable { + catalog: catalog_name.clone(), + schema: schema_name.clone(), + table: table_name.clone(), + table_id, + table_name: TableName::new(catalog_name, schema_name, table_name), + table_info, + }) + } + + async fn invalidate_table_cache_by_name( + &self, + table_id: TableId, + table_name: TableName, + ) -> Result<()> { + let cache_ident = [ + CacheIdent::TableId(table_id), + CacheIdent::TableName(table_name), + ]; + self.cache_invalidator + .invalidate(&Context::default(), &cache_ident) + .await + .context(InvalidateTableCacheSnafu)?; + Ok(()) + } } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index d71b67255383..c44a225d6cc3 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -1246,8 +1246,6 @@ pub enum SetRegionOption { Ttl(Option), // Modifying TwscOptions with values as (option name, new value). Twsc(String, String), - /// Extra table options forwarded to metadata only. - Extra(String, String), } impl TryFrom<&PbOption> for SetRegionOption { @@ -1265,7 +1263,7 @@ impl TryFrom<&PbOption> for SetRegionOption { TWCS_TRIGGER_FILE_NUM | TWCS_MAX_OUTPUT_FILE_SIZE | TWCS_TIME_WINDOW => { Ok(Self::Twsc(key.to_string(), value.to_string())) } - _ => Ok(Self::Extra(key.to_string(), value.to_string())), + _ => InvalidSetRegionOptionRequestSnafu { key, value }.fail(), } } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 64a721b8150d..3704bc889f61 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -35,8 +35,8 @@ use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, Re use crate::error::{self, Result}; use crate::requests::{ - AddColumnRequest, AlterKind, COLUMN_COMMENT_PREFIX, COMMENT_KEY, ModifyColumnTypeRequest, - SetDefaultRequest, SetIndexOption, TableOptions, UnsetIndexOption, + AddColumnRequest, AlterKind, ModifyColumnTypeRequest, SetDefaultRequest, SetIndexOption, + TableOptions, UnsetIndexOption, }; use crate::table_reference::TableReference; @@ -241,8 +241,8 @@ impl TableMeta { } // No need to rebuild table meta when renaming tables. AlterKind::RenameTable { .. } => Ok(self.new_meta_builder()), - AlterKind::SetTableOptions { options } => self.set_table_options(table_name, options), - AlterKind::UnsetTableOptions { keys } => self.unset_table_options(table_name, keys), + AlterKind::SetTableOptions { options } => self.set_table_options(options), + AlterKind::UnsetTableOptions { keys } => self.unset_table_options(keys), AlterKind::SetIndexes { options } => self.set_indexes(table_name, options), AlterKind::UnsetIndexes { options } => self.unset_indexes(table_name, options), AlterKind::DropDefaults { names } => self.drop_defaults(table_name, names), @@ -251,14 +251,8 @@ impl TableMeta { } /// Creates a [TableMetaBuilder] with modified table options. - fn set_table_options( - &self, - table_name: &str, - requests: &[SetRegionOption], - ) -> Result { + fn set_table_options(&self, requests: &[SetRegionOption]) -> Result { let mut new_options = self.options.clone(); - let table_schema = &self.schema; - let mut column_comment_updates = HashMap::new(); for request in requests { match request { @@ -280,96 +274,17 @@ impl TableMeta { new_options.extra_options.remove(key.as_str()); } } - SetRegionOption::Extra(key, value) => { - if key.starts_with(COLUMN_COMMENT_PREFIX) { - let column_name = key[COLUMN_COMMENT_PREFIX.len()..].to_string(); - let update = if value.is_empty() { - None - } else { - Some(value.to_string()) - }; - column_comment_updates.insert(column_name, update); - } else if value.is_empty() { - new_options.extra_options.remove(key); - } else { - new_options - .extra_options - .insert(key.to_string(), value.to_string()); - } - } } } - - if let Some(comment) = new_options.extra_options.get(COMMENT_KEY) - && comment.is_empty() - { - new_options.extra_options.remove(COMMENT_KEY); - } - let mut builder = self.new_meta_builder(); builder.options(new_options); - if !column_comment_updates.is_empty() { - let mut columns = Vec::with_capacity(table_schema.column_schemas().len()); - let mut remaining = column_comment_updates; - - for mut column in table_schema.column_schemas().iter().cloned() { - if let Some(update) = remaining.remove(&column.name) { - match update { - Some(comment) => { - column - .mut_metadata() - .insert(datatypes::schema::COMMENT_KEY.to_string(), comment); - } - None => { - column.mut_metadata().remove(datatypes::schema::COMMENT_KEY); - } - } - } - columns.push(column); - } - - if let Some((missing, _)) = remaining.into_iter().next() { - return error::ColumnNotExistsSnafu { - column_name: &missing, - table_name, - } - .fail(); - } - - let mut schema_builder = SchemaBuilder::try_from_columns(columns) - .with_context(|_| error::SchemaBuildSnafu { - msg: format!( - "Failed to rebuild schema for table {table_name} when updating column comments", - ), - })? - .version(table_schema.version() + 1); - - for (k, v) in table_schema.metadata().iter() { - schema_builder = schema_builder.add_metadata(k, v); - } - - let new_schema = schema_builder - .build() - .with_context(|_| error::SchemaBuildSnafu { - msg: format!("Table {table_name} cannot update column comments",), - })?; - - let _ = builder - .schema(Arc::new(new_schema)) - .primary_key_indices(self.primary_key_indices.clone()); - } - Ok(builder) } - fn unset_table_options( - &self, - table_name: &str, - requests: &[UnsetRegionOption], - ) -> Result { + fn unset_table_options(&self, requests: &[UnsetRegionOption]) -> Result { let requests = requests.iter().map(Into::into).collect::>(); - self.set_table_options(table_name, &requests) + self.set_table_options(&requests) } fn set_indexes( @@ -1983,110 +1898,6 @@ mod tests { assert_matches!(err, Error::RemovePartitionColumn { .. }); } - #[test] - fn test_set_column_comment_via_table_options() { - let schema = Arc::new(new_test_schema()); - let meta = TableMetaBuilder::empty() - .schema(schema) - .primary_key_indices(vec![0]) - .engine("engine") - .next_column_id(3) - .build() - .unwrap(); - - let alter_kind = AlterKind::SetTableOptions { - options: vec![SetRegionOption::Extra( - format!("{}{}", COLUMN_COMMENT_PREFIX, "col1"), - "first column".to_string(), - )], - }; - - let new_meta = meta - .builder_with_alter_kind("my_table", &alter_kind) - .unwrap() - .build() - .unwrap(); - - let col_schema = new_meta.schema.column_schema_by_name("col1").unwrap(); - assert_eq!( - Some(&"first column".to_string()), - col_schema.metadata().get(datatypes::schema::COMMENT_KEY) - ); - assert_eq!(meta.schema.version() + 1, new_meta.schema.version()); - } - - #[test] - fn test_remove_column_comment_via_table_options() { - let schema = Arc::new(new_test_schema()); - let meta = TableMetaBuilder::empty() - .schema(schema) - .primary_key_indices(vec![0]) - .engine("engine") - .next_column_id(3) - .build() - .unwrap(); - - let add_comment = AlterKind::SetTableOptions { - options: vec![SetRegionOption::Extra( - format!("{}{}", COLUMN_COMMENT_PREFIX, "col1"), - "first column".to_string(), - )], - }; - let meta_with_comment = meta - .builder_with_alter_kind("my_table", &add_comment) - .unwrap() - .build() - .unwrap(); - - let remove_comment = AlterKind::SetTableOptions { - options: vec![SetRegionOption::Extra( - format!("{}{}", COLUMN_COMMENT_PREFIX, "col1"), - String::new(), - )], - }; - let meta_without_comment = meta_with_comment - .builder_with_alter_kind("my_table", &remove_comment) - .unwrap() - .build() - .unwrap(); - - let col_schema = meta_without_comment - .schema - .column_schema_by_name("col1") - .unwrap(); - assert!( - col_schema - .metadata() - .get(datatypes::schema::COMMENT_KEY) - .is_none() - ); - } - - #[test] - fn test_set_column_comment_unknown_column() { - let schema = Arc::new(new_test_schema()); - let meta = TableMetaBuilder::empty() - .schema(schema) - .primary_key_indices(vec![0]) - .engine("engine") - .next_column_id(3) - .build() - .unwrap(); - - let alter_kind = AlterKind::SetTableOptions { - options: vec![SetRegionOption::Extra( - format!("{}{}", COLUMN_COMMENT_PREFIX, "unknown"), - "comment".to_string(), - )], - }; - - let err = meta - .builder_with_alter_kind("my_table", &alter_kind) - .err() - .unwrap(); - assert_eq!(StatusCode::TableColumnNotFound, err.status_code()); - } - #[test] fn test_change_key_column_data_type() { let schema = Arc::new(new_test_schema()); diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index ad13e97b7df2..8f64f0b9fc8f 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -135,7 +135,6 @@ pub const WRITE_BUFFER_SIZE_KEY: &str = "write_buffer_size"; pub const TTL_KEY: &str = store_api::mito_engine_options::TTL_KEY; pub const STORAGE_KEY: &str = "storage"; pub const COMMENT_KEY: &str = "comment"; -pub const COLUMN_COMMENT_PREFIX: &str = "column.comment."; pub const AUTO_CREATE_TABLE_KEY: &str = "auto_create_table"; pub const SKIP_WAL_KEY: &str = store_api::mito_engine_options::SKIP_WAL_KEY; From 68266fe7883379d5000d4e69e0f451c38372908f Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Wed, 24 Sep 2025 08:51:56 +0800 Subject: [PATCH 03/21] sqlness test Signed-off-by: Ruihang Xia --- tests/cases/standalone/common/comment.sql | 65 +++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 tests/cases/standalone/common/comment.sql diff --git a/tests/cases/standalone/common/comment.sql b/tests/cases/standalone/common/comment.sql new file mode 100644 index 000000000000..071a645dd763 --- /dev/null +++ b/tests/cases/standalone/common/comment.sql @@ -0,0 +1,65 @@ +-- Test: COMMENT ON TABLE add & remove +CREATE TABLE comment_table_test ( + pk INT, + val DOUBLE, + ts TIMESTAMP TIME INDEX, + PRIMARY KEY(pk) +); + +-- Add table comment +COMMENT ON TABLE comment_table_test IS 'table level description'; +SHOW CREATE TABLE comment_table_test; + +-- Remove table comment +COMMENT ON TABLE comment_table_test IS NULL; +SHOW CREATE TABLE comment_table_test; + +DROP TABLE comment_table_test; + +-- Test: COMMENT ON COLUMN add & remove +CREATE TABLE comment_column_test ( + pk INT, + val DOUBLE, + ts TIMESTAMP TIME INDEX, + PRIMARY KEY(pk) +); + +-- Add column comment +COMMENT ON COLUMN comment_column_test.val IS 'value column description'; +SHOW CREATE TABLE comment_column_test; + +-- Remove column comment +COMMENT ON COLUMN comment_column_test.val IS NULL; +SHOW CREATE TABLE comment_column_test; + +DROP TABLE comment_column_test; + +-- Test: COMMENT ON FLOW add & remove +-- Prepare base & sink tables +CREATE TABLE flow_base_comment_test ( + desc_str STRING, + ts TIMESTAMP TIME INDEX +); + +CREATE TABLE flow_sink_comment_test ( + desc_str STRING, + ts TIMESTAMP TIME INDEX +); + +CREATE FLOW flow_comment_test +SINK TO flow_sink_comment_test +AS +SELECT desc_str, ts FROM flow_base_comment_test; + +-- Add flow comment +COMMENT ON FLOW flow_comment_test IS 'flow level description'; +SHOW CREATE FLOW flow_comment_test; + +-- Remove flow comment +COMMENT ON FLOW flow_comment_test IS NULL; +SHOW CREATE FLOW flow_comment_test; + +DROP FLOW flow_comment_test; +DROP TABLE flow_base_comment_test; +DROP TABLE flow_sink_comment_test; + From bb5f7f7b48090454865e8f050a2c529bc99e795a Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 13:07:29 +0800 Subject: [PATCH 04/21] avoid unimplemented panic Signed-off-by: Ruihang Xia --- src/catalog/src/kvbackend/client.rs | 16 ++- src/operator/src/statement/comment.rs | 162 +++++++++++++++++++++++--- 2 files changed, 161 insertions(+), 17 deletions(-) diff --git a/src/catalog/src/kvbackend/client.rs b/src/catalog/src/kvbackend/client.rs index f74509217fb0..0768ac5e9dad 100644 --- a/src/catalog/src/kvbackend/client.rs +++ b/src/catalog/src/kvbackend/client.rs @@ -21,7 +21,9 @@ use std::time::Duration; use common_error::ext::BoxedError; use common_meta::cache_invalidator::KvCacheInvalidator; use common_meta::error::Error::CacheNotGet; -use common_meta::error::{CacheNotGetSnafu, Error, ExternalSnafu, GetKvCacheSnafu, Result}; +use common_meta::error::{ + CacheNotGetSnafu, Error, ExternalSnafu, GetKvCacheSnafu, Result, UnsupportedSnafu, +}; use common_meta::kv_backend::txn::{Txn, TxnResponse}; use common_meta::kv_backend::{KvBackend, KvBackendRef, TxnService}; use common_meta::rpc::KeyValue; @@ -368,8 +370,20 @@ impl MetaKvBackend { } } +#[async_trait::async_trait] impl TxnService for MetaKvBackend { type Error = Error; + + async fn txn(&self, _txn: Txn) -> std::result::Result { + UnsupportedSnafu { + operation: "MetaKvBackend::txn", + } + .fail() + } + + fn max_txn_ops(&self) -> usize { + 0 + } } /// Implement `KvBackend` trait for `MetaKvBackend` instead of opendal's `Accessor` since diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index b8b415744f03..3fb6c8fa8251 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -12,20 +12,28 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::sync::Arc; + use chrono::Utc; use common_catalog::format_full_table_name; use common_error::ext::BoxedError; use common_meta::cache_invalidator::Context; +use common_meta::error::Error as MetaError; use common_meta::instruction::CacheIdent; -use common_meta::key::DeserializedValueWithBytes; -use common_meta::key::table_info::TableInfoValue; +use common_meta::key::flow::flow_info::{FlowInfoKey, FlowInfoValue}; +use common_meta::key::flow::flow_route::FlowRouteValue; +use common_meta::key::table_info::{TableInfoKey, TableInfoValue}; +use common_meta::key::{DeserializedValueWithBytes, FlowId, FlowPartitionId, MetadataKey}; +use common_meta::rpc::store::PutRequest; use common_query::Output; +use common_telemetry::debug; +use serde_json::{Map, Value}; use session::context::QueryContextRef; use session::table_name::table_idents_to_full_name; -use snafu::{OptionExt, ResultExt}; +use snafu::{GenerateImplicitData, Location, OptionExt, ResultExt}; use sql::ast::{Ident, ObjectName, ObjectNamePartExt}; use sql::statements::comment::{Comment, CommentObject}; -use table::metadata::TableId; +use table::metadata::{RawTableInfo, TableId}; use table::requests::COMMENT_KEY; use table::table_name::TableName; @@ -74,10 +82,8 @@ impl StatementExecutor { let mut new_table_info = table_info.table_info.clone(); new_table_info.desc = comment; - self.table_metadata_manager - .update_table_info(&table_info, None, new_table_info) - .await - .context(TableMetadataManagerSnafu)?; + self.update_table_info_with_fallback(&table_info, new_table_info) + .await?; self.invalidate_table_cache_by_name(table_id, table_name) .await?; @@ -126,10 +132,8 @@ impl StatementExecutor { } } - self.table_metadata_manager - .update_table_info(&table_info, None, new_table_info) - .await - .context(TableMetadataManagerSnafu)?; + self.update_table_info_with_fallback(&table_info, new_table_info) + .await?; self.invalidate_table_cache_by_name(table_id, table_name) .await?; @@ -194,10 +198,8 @@ impl StatementExecutor { .map(|(key, value)| (key.partition_id(), value)) .collect::>(); - self.flow_metadata_manager - .update_flow_metadata(flow_id, &flow_info, &new_flow_info, flow_routes) - .await - .context(TableMetadataManagerSnafu)?; + self.update_flow_metadata_with_fallback(flow_id, &flow_info, new_flow_info, flow_routes) + .await?; Ok(Output::new_with_affected_rows(0)) } @@ -258,4 +260,132 @@ impl StatementExecutor { .context(InvalidateTableCacheSnafu)?; Ok(()) } + + async fn update_table_info_with_fallback( + &self, + current_table_info: &DeserializedValueWithBytes, + new_table_info: RawTableInfo, + ) -> Result<()> { + let try_update = self + .table_metadata_manager + .update_table_info(current_table_info, None, new_table_info.clone()) + .await + .context(TableMetadataManagerSnafu); + + match try_update { + Ok(()) => Ok(()), + Err(err) if Self::is_txn_unsupported(&err) => { + debug!("comment update_table_info fallback due to unsupported txn capability"); + self.update_table_info_without_txn(current_table_info, new_table_info) + .await + } + Err(err) => Err(err), + } + } + + async fn update_flow_metadata_with_fallback( + &self, + flow_id: FlowId, + current_flow_info: &DeserializedValueWithBytes, + new_flow_info: FlowInfoValue, + flow_routes: Vec<(FlowPartitionId, FlowRouteValue)>, + ) -> Result<()> { + let try_update = self + .flow_metadata_manager + .update_flow_metadata(flow_id, current_flow_info, &new_flow_info, flow_routes) + .await + .context(TableMetadataManagerSnafu); + + match try_update { + Ok(()) => Ok(()), + Err(err) if Self::is_txn_unsupported(&err) => { + debug!("comment update_flow fallback due to unsupported txn capability"); + self.update_flow_metadata_without_txn(flow_id, new_flow_info) + .await + } + Err(err) => Err(err), + } + } + + async fn update_table_info_without_txn( + &self, + current_table_info: &DeserializedValueWithBytes, + new_table_info: RawTableInfo, + ) -> Result<()> { + let kv_backend = Arc::clone(self.table_metadata_manager.kv_backend()); + let table_id = current_table_info.table_info.ident.table_id; + let convert_err = |err| common_meta::error::Error::SerdeJson { + error: err, + location: Location::generate(), + }; + + let mut stored_obj: Map = + serde_json::from_slice(¤t_table_info.get_raw_bytes()) + .map_err(convert_err) + .context(TableMetadataManagerSnafu)?; + + let current_version = stored_obj + .get("version") + .and_then(Value::as_u64) + .unwrap_or_default(); + stored_obj.insert( + "version".to_string(), + Value::from(current_version.saturating_add(1)), + ); + stored_obj.insert( + "table_info".to_string(), + serde_json::to_value(&new_table_info) + .map_err(convert_err) + .context(TableMetadataManagerSnafu)?, + ); + + let raw_value = serde_json::to_vec(&stored_obj) + .map_err(convert_err) + .context(TableMetadataManagerSnafu)?; + + kv_backend + .put( + PutRequest::new() + .with_key(TableInfoKey::new(table_id).to_bytes()) + .with_value(raw_value), + ) + .await + .context(TableMetadataManagerSnafu)?; + + Ok(()) + } + + async fn update_flow_metadata_without_txn( + &self, + flow_id: FlowId, + new_flow_info: FlowInfoValue, + ) -> Result<()> { + let kv_backend = Arc::clone(self.table_metadata_manager.kv_backend()); + let raw_value = serde_json::to_vec(&new_flow_info) + .map_err(|err| common_meta::error::Error::SerdeJson { + error: err, + location: Location::generate(), + }) + .context(TableMetadataManagerSnafu)?; + + kv_backend + .put( + PutRequest::new() + .with_key(FlowInfoKey::new(flow_id).to_bytes()) + .with_value(raw_value), + ) + .await + .context(TableMetadataManagerSnafu)?; + + Ok(()) + } + + fn is_txn_unsupported(error: &crate::error::Error) -> bool { + matches!( + error, + crate::error::Error::TableMetadataManager { source, .. } + if matches!(source, MetaError::Unsupported { operation, .. } + if operation == "txn" || operation.ends_with("::txn")) + ) + } } From acf5bca735279b526099ae19554ecd6062db5e3a Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 13:08:31 +0800 Subject: [PATCH 05/21] validate flow Signed-off-by: Ruihang Xia --- src/frontend/src/instance.rs | 49 +++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/src/frontend/src/instance.rs b/src/frontend/src/instance.rs index 406395ce7087..71d2f0f5747b 100644 --- a/src/frontend/src/instance.rs +++ b/src/frontend/src/instance.rs @@ -876,7 +876,7 @@ pub fn check_permission( validate_param(&stmt.table_name, query_ctx)?; } Statement::ShowCreateFlow(stmt) => { - validate_param(&stmt.flow_name, query_ctx)?; + validate_flow(&stmt.flow_name, query_ctx)?; } Statement::ShowCreateView(stmt) => { validate_param(&stmt.view_name, query_ctx)?; @@ -908,7 +908,7 @@ pub fn check_permission( Statement::Comment(comment) => match &comment.object { CommentObject::Table(table) => validate_param(table, query_ctx)?, CommentObject::Column { table, .. } => validate_param(table, query_ctx)?, - CommentObject::Flow(flow) => validate_param(flow, query_ctx)?, + CommentObject::Flow(flow) => validate_flow(flow, query_ctx)?, }, Statement::Insert(insert) => { @@ -996,6 +996,27 @@ fn validate_param(name: &ObjectName, query_ctx: &QueryContextRef) -> Result<()> .context(SqlExecInterceptedSnafu) } +fn validate_flow(name: &ObjectName, query_ctx: &QueryContextRef) -> Result<()> { + let catalog = match &name.0[..] { + [_flow] => query_ctx.current_catalog().to_string(), + [catalog, _flow] => catalog.to_string_unquoted(), + _ => { + return InvalidSqlSnafu { + err_msg: format!( + "expect flow name to be . or , actual: {name}", + ), + } + .fail(); + } + }; + + let schema = query_ctx.current_schema(); + + validate_catalog_and_schema(&catalog, &schema, query_ctx) + .map_err(BoxedError::new) + .context(SqlExecInterceptedSnafu) +} + fn validate_database(name: &ObjectName, query_ctx: &QueryContextRef) -> Result<()> { let (catalog, schema) = match &name.0[..] { [schema] => ( @@ -1254,6 +1275,28 @@ mod tests { // test describe table let sql = "DESC TABLE {catalog}{schema}demo;"; - replace_test(sql, plugins, &query_ctx); + replace_test(sql, plugins.clone(), &query_ctx); + + let comment_flow_cases = [ + ("COMMENT ON FLOW my_flow IS 'comment';", true), + ("COMMENT ON FLOW greptime.my_flow IS 'comment';", true), + ("COMMENT ON FLOW wrongcatalog.my_flow IS 'comment';", false), + ]; + for (sql, is_ok) in comment_flow_cases { + let stmt = &parse_stmt(sql, &GreptimeDbDialect {}).unwrap()[0]; + let result = check_permission(plugins.clone(), stmt, &query_ctx); + assert_eq!(result.is_ok(), is_ok); + } + + let show_flow_cases = [ + ("SHOW CREATE FLOW my_flow;", true), + ("SHOW CREATE FLOW greptime.my_flow;", true), + ("SHOW CREATE FLOW wrongcatalog.my_flow;", false), + ]; + for (sql, is_ok) in show_flow_cases { + let stmt = &parse_stmt(sql, &GreptimeDbDialect {}).unwrap()[0]; + let result = check_permission(plugins.clone(), stmt, &query_ctx); + assert_eq!(result.is_ok(), is_ok); + } } } From b89ec2c35377b662da696654ba7f22b19acad33c Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 13:08:40 +0800 Subject: [PATCH 06/21] update sqlness result Signed-off-by: Ruihang Xia --- tests/cases/standalone/common/comment.result | 182 +++++++++++++++++++ 1 file changed, 182 insertions(+) create mode 100644 tests/cases/standalone/common/comment.result diff --git a/tests/cases/standalone/common/comment.result b/tests/cases/standalone/common/comment.result new file mode 100644 index 000000000000..44c1df167461 --- /dev/null +++ b/tests/cases/standalone/common/comment.result @@ -0,0 +1,182 @@ +-- Test: COMMENT ON TABLE add & remove +CREATE TABLE comment_table_test ( + pk INT, + val DOUBLE, + ts TIMESTAMP TIME INDEX, + PRIMARY KEY(pk) +); + +Affected Rows: 0 + +-- Add table comment +COMMENT ON TABLE comment_table_test IS 'table level description'; + +Affected Rows: 0 + +SHOW CREATE TABLE comment_table_test; + ++--------------------+---------------------------------------------------+ +| Table | Create Table | ++--------------------+---------------------------------------------------+ +| comment_table_test | CREATE TABLE IF NOT EXISTS "comment_table_test" ( | +| | "pk" INT NULL, | +| | "val" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("pk") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++--------------------+---------------------------------------------------+ + +-- Remove table comment +COMMENT ON TABLE comment_table_test IS NULL; + +Affected Rows: 0 + +SHOW CREATE TABLE comment_table_test; + ++--------------------+---------------------------------------------------+ +| Table | Create Table | ++--------------------+---------------------------------------------------+ +| comment_table_test | CREATE TABLE IF NOT EXISTS "comment_table_test" ( | +| | "pk" INT NULL, | +| | "val" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("pk") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++--------------------+---------------------------------------------------+ + +DROP TABLE comment_table_test; + +Affected Rows: 0 + +-- Test: COMMENT ON COLUMN add & remove +CREATE TABLE comment_column_test ( + pk INT, + val DOUBLE, + ts TIMESTAMP TIME INDEX, + PRIMARY KEY(pk) +); + +Affected Rows: 0 + +-- Add column comment +COMMENT ON COLUMN comment_column_test.val IS 'value column description'; + +Affected Rows: 0 + +SHOW CREATE TABLE comment_column_test; + ++---------------------+----------------------------------------------------+ +| Table | Create Table | ++---------------------+----------------------------------------------------+ +| comment_column_test | CREATE TABLE IF NOT EXISTS "comment_column_test" ( | +| | "pk" INT NULL, | +| | "val" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("pk") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++---------------------+----------------------------------------------------+ + +-- Remove column comment +COMMENT ON COLUMN comment_column_test.val IS NULL; + +Affected Rows: 0 + +SHOW CREATE TABLE comment_column_test; + ++---------------------+----------------------------------------------------+ +| Table | Create Table | ++---------------------+----------------------------------------------------+ +| comment_column_test | CREATE TABLE IF NOT EXISTS "comment_column_test" ( | +| | "pk" INT NULL, | +| | "val" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("pk") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++---------------------+----------------------------------------------------+ + +DROP TABLE comment_column_test; + +Affected Rows: 0 + +-- Test: COMMENT ON FLOW add & remove +-- Prepare base & sink tables +CREATE TABLE flow_base_comment_test ( + desc_str STRING, + ts TIMESTAMP TIME INDEX +); + +Affected Rows: 0 + +CREATE TABLE flow_sink_comment_test ( + desc_str STRING, + ts TIMESTAMP TIME INDEX +); + +Affected Rows: 0 + +CREATE FLOW flow_comment_test +SINK TO flow_sink_comment_test +AS +SELECT desc_str, ts FROM flow_base_comment_test; + +Affected Rows: 0 + +-- Add flow comment +COMMENT ON FLOW flow_comment_test IS 'flow level description'; + +Affected Rows: 0 + +SHOW CREATE FLOW flow_comment_test; + ++-------------------+----------------------------------------------------+ +| Flow | Create Flow | ++-------------------+----------------------------------------------------+ +| flow_comment_test | CREATE FLOW IF NOT EXISTS flow_comment_test | +| | SINK TO flow_sink_comment_test | +| | COMMENT 'flow level description' | +| | AS SELECT desc_str, ts FROM flow_base_comment_test | ++-------------------+----------------------------------------------------+ + +-- Remove flow comment +COMMENT ON FLOW flow_comment_test IS NULL; + +Affected Rows: 0 + +SHOW CREATE FLOW flow_comment_test; + ++-------------------+----------------------------------------------------+ +| Flow | Create Flow | ++-------------------+----------------------------------------------------+ +| flow_comment_test | CREATE FLOW IF NOT EXISTS flow_comment_test | +| | SINK TO flow_sink_comment_test | +| | AS SELECT desc_str, ts FROM flow_base_comment_test | ++-------------------+----------------------------------------------------+ + +DROP FLOW flow_comment_test; + +Affected Rows: 0 + +DROP TABLE flow_base_comment_test; + +Affected Rows: 0 + +DROP TABLE flow_sink_comment_test; + +Affected Rows: 0 + From 658274e15a52b6b6124868cbf10b1216fc8d517d Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 15:12:05 +0800 Subject: [PATCH 07/21] fix table column comment Signed-off-by: Ruihang Xia --- src/operator/src/statement/comment.rs | 58 ++++++++++++++++---- tests/cases/standalone/common/comment.result | 28 +++++----- tests/cases/standalone/common/comment.sql | 8 +-- 3 files changed, 65 insertions(+), 29 deletions(-) diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index 3fb6c8fa8251..2090dc1114f1 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -27,6 +27,7 @@ use common_meta::key::{DeserializedValueWithBytes, FlowId, FlowPartitionId, Meta use common_meta::rpc::store::PutRequest; use common_query::Output; use common_telemetry::debug; +use datatypes::schema::COMMENT_KEY; use serde_json::{Map, Value}; use session::context::QueryContextRef; use session::table_name::table_idents_to_full_name; @@ -34,7 +35,6 @@ use snafu::{GenerateImplicitData, Location, OptionExt, ResultExt}; use sql::ast::{Ident, ObjectName, ObjectNamePartExt}; use sql::statements::comment::{Comment, CommentObject}; use table::metadata::{RawTableInfo, TableId}; -use table::requests::COMMENT_KEY; use table::table_name::TableName; use crate::error::{ @@ -121,16 +121,7 @@ impl StatementExecutor { msg: format!("{} column `{}`", full_table_name, column_name), })?; - match comment { - Some(value) => { - column_schema - .mut_metadata() - .insert(COMMENT_KEY.to_string(), value); - } - None => { - column_schema.mut_metadata().remove(COMMENT_KEY); - } - } + update_column_comment_metadata(column_schema, comment); self.update_table_info_with_fallback(&table_info, new_table_info) .await?; @@ -389,3 +380,48 @@ impl StatementExecutor { ) } } + +fn update_column_comment_metadata( + column_schema: &mut datatypes::schema::ColumnSchema, + comment: Option, +) { + match comment { + Some(value) => { + column_schema + .mut_metadata() + .insert(COMMENT_KEY.to_string(), value); + } + None => { + column_schema.mut_metadata().remove(COMMENT_KEY); + } + } +} + +#[cfg(test)] +mod tests { + use datatypes::data_type::ConcreteDataType; + use datatypes::schema::{COMMENT_KEY, ColumnSchema}; + + use super::update_column_comment_metadata; + + #[test] + fn test_update_column_comment_metadata_uses_schema_key() { + let mut column_schema = ColumnSchema::new("col", ConcreteDataType::string_datatype(), true); + + update_column_comment_metadata(&mut column_schema, Some("note".to_string())); + + assert_eq!( + column_schema + .metadata() + .get(COMMENT_KEY) + .map(|value| value.as_str()), + Some("note") + ); + // Make sure it's not the `COMMENT_KEY` from `table::request` + assert!(column_schema.metadata().get("comment").is_none()); + + update_column_comment_metadata(&mut column_schema, None); + + assert!(column_schema.metadata().get(COMMENT_KEY).is_none()); + } +} diff --git a/tests/cases/standalone/common/comment.result b/tests/cases/standalone/common/comment.result index 44c1df167461..b5e186f48304 100644 --- a/tests/cases/standalone/common/comment.result +++ b/tests/cases/standalone/common/comment.result @@ -73,20 +73,20 @@ Affected Rows: 0 SHOW CREATE TABLE comment_column_test; -+---------------------+----------------------------------------------------+ -| Table | Create Table | -+---------------------+----------------------------------------------------+ -| comment_column_test | CREATE TABLE IF NOT EXISTS "comment_column_test" ( | -| | "pk" INT NULL, | -| | "val" DOUBLE NULL, | -| | "ts" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("ts"), | -| | PRIMARY KEY ("pk") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+---------------------+----------------------------------------------------+ ++---------------------+---------------------------------------------------------+ +| Table | Create Table | ++---------------------+---------------------------------------------------------+ +| comment_column_test | CREATE TABLE IF NOT EXISTS "comment_column_test" ( | +| | "pk" INT NULL, | +| | "val" DOUBLE NULL COMMENT 'value column description', | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("pk") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++---------------------+---------------------------------------------------------+ -- Remove column comment COMMENT ON COLUMN comment_column_test.val IS NULL; diff --git a/tests/cases/standalone/common/comment.sql b/tests/cases/standalone/common/comment.sql index 071a645dd763..564480563bde 100644 --- a/tests/cases/standalone/common/comment.sql +++ b/tests/cases/standalone/common/comment.sql @@ -35,8 +35,8 @@ SHOW CREATE TABLE comment_column_test; DROP TABLE comment_column_test; -- Test: COMMENT ON FLOW add & remove --- Prepare base & sink tables -CREATE TABLE flow_base_comment_test ( +-- Prepare source & sink tables +CREATE TABLE flow_source_comment_test ( desc_str STRING, ts TIMESTAMP TIME INDEX ); @@ -49,7 +49,7 @@ CREATE TABLE flow_sink_comment_test ( CREATE FLOW flow_comment_test SINK TO flow_sink_comment_test AS -SELECT desc_str, ts FROM flow_base_comment_test; +SELECT desc_str, ts FROM flow_source_comment_test; -- Add flow comment COMMENT ON FLOW flow_comment_test IS 'flow level description'; @@ -60,6 +60,6 @@ COMMENT ON FLOW flow_comment_test IS NULL; SHOW CREATE FLOW flow_comment_test; DROP FLOW flow_comment_test; -DROP TABLE flow_base_comment_test; +DROP TABLE flow_source_comment_test; DROP TABLE flow_sink_comment_test; From 3ee820a50e6eda4c74d055ba561121d0794e7944 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 18:07:57 +0800 Subject: [PATCH 08/21] table level comment Signed-off-by: Ruihang Xia --- src/operator/src/statement/comment.rs | 50 ++- src/operator/src/statement/show.rs | 35 ++- src/query/src/sql.rs | 8 +- src/query/src/sql/show_create_table.rs | 13 +- tests-integration/src/grpc.rs | 1 + tests-integration/tests/grpc.rs | 35 ++- tests-integration/tests/http.rs | 38 ++- .../distributed/flow-tql/flow_tql.result | 68 ++-- tests/cases/standalone/common/comment.result | 42 +-- .../common/flow/flow_advance_ttl.result | 9 +- .../common/flow/flow_advance_ttl.sql | 1 + .../common/flow/flow_auto_sink_table.result | 112 ++++--- .../standalone/common/flow/flow_basic.result | 296 ++++++++++-------- .../cases/standalone/flow-tql/flow_tql.result | 68 ++-- 14 files changed, 456 insertions(+), 320 deletions(-) diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index 2090dc1114f1..e1bd82351b2b 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -26,8 +26,7 @@ use common_meta::key::table_info::{TableInfoKey, TableInfoValue}; use common_meta::key::{DeserializedValueWithBytes, FlowId, FlowPartitionId, MetadataKey}; use common_meta::rpc::store::PutRequest; use common_query::Output; -use common_telemetry::debug; -use datatypes::schema::COMMENT_KEY; +use datatypes::schema::COMMENT_KEY as COLUMN_COMMENT_KEY; use serde_json::{Map, Value}; use session::context::QueryContextRef; use session::table_name::table_idents_to_full_name; @@ -35,6 +34,7 @@ use snafu::{GenerateImplicitData, Location, OptionExt, ResultExt}; use sql::ast::{Ident, ObjectName, ObjectNamePartExt}; use sql::statements::comment::{Comment, CommentObject}; use table::metadata::{RawTableInfo, TableId}; +use table::requests::COMMENT_KEY as TABLE_COMMENT_KEY; use table::table_name::TableName; use crate::error::{ @@ -81,6 +81,10 @@ impl StatementExecutor { let mut new_table_info = table_info.table_info.clone(); new_table_info.desc = comment; + sync_table_comment_option( + &mut new_table_info.meta.options, + new_table_info.desc.as_deref(), + ); self.update_table_info_with_fallback(&table_info, new_table_info) .await?; @@ -266,7 +270,6 @@ impl StatementExecutor { match try_update { Ok(()) => Ok(()), Err(err) if Self::is_txn_unsupported(&err) => { - debug!("comment update_table_info fallback due to unsupported txn capability"); self.update_table_info_without_txn(current_table_info, new_table_info) .await } @@ -290,7 +293,6 @@ impl StatementExecutor { match try_update { Ok(()) => Ok(()), Err(err) if Self::is_txn_unsupported(&err) => { - debug!("comment update_flow fallback due to unsupported txn capability"); self.update_flow_metadata_without_txn(flow_id, new_flow_info) .await } @@ -389,10 +391,23 @@ fn update_column_comment_metadata( Some(value) => { column_schema .mut_metadata() - .insert(COMMENT_KEY.to_string(), value); + .insert(COLUMN_COMMENT_KEY.to_string(), value); + } + None => { + column_schema.mut_metadata().remove(COLUMN_COMMENT_KEY); + } + } +} + +fn sync_table_comment_option(options: &mut table::requests::TableOptions, comment: Option<&str>) { + match comment { + Some(value) => { + options + .extra_options + .insert(TABLE_COMMENT_KEY.to_string(), value.to_string()); } None => { - column_schema.mut_metadata().remove(COMMENT_KEY); + options.extra_options.remove(TABLE_COMMENT_KEY); } } } @@ -400,9 +415,10 @@ fn update_column_comment_metadata( #[cfg(test)] mod tests { use datatypes::data_type::ConcreteDataType; - use datatypes::schema::{COMMENT_KEY, ColumnSchema}; + use datatypes::schema::{COMMENT_KEY as COLUMN_COMMENT_KEY, ColumnSchema}; + use table::requests::TableOptions; - use super::update_column_comment_metadata; + use super::{sync_table_comment_option, update_column_comment_metadata}; #[test] fn test_update_column_comment_metadata_uses_schema_key() { @@ -413,7 +429,7 @@ mod tests { assert_eq!( column_schema .metadata() - .get(COMMENT_KEY) + .get(COLUMN_COMMENT_KEY) .map(|value| value.as_str()), Some("note") ); @@ -422,6 +438,20 @@ mod tests { update_column_comment_metadata(&mut column_schema, None); - assert!(column_schema.metadata().get(COMMENT_KEY).is_none()); + assert!(column_schema.metadata().get(COLUMN_COMMENT_KEY).is_none()); + } + + #[test] + fn test_sync_table_comment_option() { + let mut options = TableOptions::default(); + + sync_table_comment_option(&mut options, Some("table comment")); + assert_eq!( + options.extra_options.get(super::TABLE_COMMENT_KEY), + Some(&"table comment".to_string()) + ); + + sync_table_comment_option(&mut options, None); + assert!(!options.extra_options.contains_key(super::TABLE_COMMENT_KEY)); } } diff --git a/src/operator/src/statement/show.rs b/src/operator/src/statement/show.rs index b98ff1dab537..65c6801a0dba 100644 --- a/src/operator/src/statement/show.rs +++ b/src/operator/src/statement/show.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::sync::Arc; + use common_error::ext::BoxedError; use common_meta::key::schema_name::SchemaNameKey; use common_query::Output; @@ -120,7 +122,36 @@ impl StatementExecutor { table: TableRef, query_ctx: QueryContextRef, ) -> Result { - let table_info = table.table_info(); + let table_info_with_partition_indices = table.table_info(); + let partition_column_names: Vec<_> = table_info_with_partition_indices + .meta + .partition_column_names() + .cloned() + .collect(); + let mut table_info = table_info_with_partition_indices; + + if let Some(latest) = self + .table_metadata_manager + .table_info_manager() + .get(table_info.table_id()) + .await + .context(TableMetadataManagerSnafu)? + { + let mut latest_info = TableInfo::try_from(latest.into_inner().table_info) + .context(error::CreateTableInfoSnafu)?; + + if !partition_column_names.is_empty() { + latest_info.meta.partition_key_indices = partition_column_names + .iter() + .filter_map(|name| { + latest_info.meta.schema.column_index_by_name(name.as_str()) + }) + .collect(); + } + + table_info = Arc::new(latest_info); + } + if table_info.table_type != TableType::Base { return error::ShowCreateTableBaseOnlySnafu { table_name: table_name.to_string(), @@ -150,7 +181,7 @@ impl StatementExecutor { let partitions = create_partitions_stmt(&table_info, partitions)?; - query::sql::show_create_table(table, schema_options, partitions, query_ctx) + query::sql::show_create_table(table_info, schema_options, partitions, query_ctx) .context(ExecuteStatementSnafu) } diff --git a/src/query/src/sql.rs b/src/query/src/sql.rs index 6b6ee2ed07e5..2caccef8d7b5 100644 --- a/src/query/src/sql.rs +++ b/src/query/src/sql.rs @@ -64,6 +64,7 @@ use sql::statements::statement::Statement; use sqlparser::ast::ObjectName; use store_api::metric_engine_consts::{is_metric_engine, is_metric_engine_internal_column}; use table::TableRef; +use table::metadata::TableInfoRef; use table::requests::{FILE_TABLE_LOCATION_KEY, FILE_TABLE_PATTERN_KEY}; use crate::QueryEngineRef; @@ -789,13 +790,12 @@ pub fn show_create_database(database_name: &str, options: OptionMap) -> Result, partitions: Option, query_ctx: QueryContextRef, ) -> Result { - let table_info = table.table_info(); - let table_name = &table_info.name; + let table_name = table_info.name.clone(); let quote_style = query_ctx.quote_style(); @@ -806,7 +806,7 @@ pub fn show_create_table( }); let sql = format!("{}", stmt); let columns = vec![ - Arc::new(StringVector::from(vec![table_name.clone()])) as _, + Arc::new(StringVector::from(vec![table_name])) as _, Arc::new(StringVector::from(vec![sql])) as _, ]; let records = RecordBatches::try_from_columns(SHOW_CREATE_TABLE_OUTPUT_SCHEMA.clone(), columns) diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs index 4981edeef7ed..3efccc0c5a17 100644 --- a/src/query/src/sql/show_create_table.rs +++ b/src/query/src/sql/show_create_table.rs @@ -32,7 +32,9 @@ use sql::statements::create::{Column, ColumnExtensions, CreateTable, TableConstr use sql::statements::{self, OptionMap}; use store_api::metric_engine_consts::{is_metric_engine, is_metric_engine_internal_column}; use table::metadata::{TableInfoRef, TableMeta}; -use table::requests::{FILE_TABLE_META_KEY, TTL_KEY, WRITE_BUFFER_SIZE_KEY}; +use table::requests::{ + COMMENT_KEY as TABLE_COMMENT_KEY, FILE_TABLE_META_KEY, TTL_KEY, WRITE_BUFFER_SIZE_KEY, +}; use crate::error::{ ConvertSqlTypeSnafu, ConvertSqlValueSnafu, GetFulltextOptionsSnafu, @@ -238,6 +240,13 @@ pub fn create_table_stmt( let constraints = create_table_constraints(&table_meta.engine, schema, table_meta, quote_style); + let mut options = create_sql_options(table_meta, schema_options); + if let Some(comment) = &table_info.desc { + if options.get(TABLE_COMMENT_KEY).is_none() { + options.insert(format!("'{TABLE_COMMENT_KEY}'"), comment.clone()); + } + } + Ok(CreateTable { if_not_exists: true, table_id: table_info.ident.table_id, @@ -245,7 +254,7 @@ pub fn create_table_stmt( columns, engine: table_meta.engine.clone(), constraints, - options: create_sql_options(table_meta, schema_options), + options, partitions: None, }) } diff --git a/tests-integration/src/grpc.rs b/tests-integration/src/grpc.rs index 6a5efda46a44..a6e4cf6f6dd0 100644 --- a/tests-integration/src/grpc.rs +++ b/tests-integration/src/grpc.rs @@ -1255,6 +1255,7 @@ CREATE TABLE {table_name} ( | | | | | ENGINE=mito | | | WITH( | +| | 'comment' = 'Created on insertion', | | | 'compaction.twcs.time_window' = '1d', | | | 'compaction.type' = 'twcs' | | | ) | diff --git a/tests-integration/tests/grpc.rs b/tests-integration/tests/grpc.rs index 1ee3e4d09b20..519cd7bc640a 100644 --- a/tests-integration/tests/grpc.rs +++ b/tests-integration/tests/grpc.rs @@ -515,23 +515,24 @@ async fn insert_with_hints_and_assert(db: &Database) { let pretty = record_batches.pretty_print().unwrap(); let expected = "\ -+-------+-------------------------------------+ -| Table | Create Table | -+-------+-------------------------------------+ -| demo | CREATE TABLE IF NOT EXISTS \"demo\" ( | -| | \"host\" STRING NULL, | -| | \"cpu\" DOUBLE NULL, | -| | \"memory\" DOUBLE NULL, | -| | \"ts\" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX (\"ts\"), | -| | PRIMARY KEY (\"host\") | -| | ) | -| | | -| | ENGINE=mito | -| | WITH( | -| | append_mode = 'true' | -| | ) | -+-------+-------------------------------------+\ ++-------+---------------------------------------+ +| Table | Create Table | ++-------+---------------------------------------+ +| demo | CREATE TABLE IF NOT EXISTS \"demo\" ( | +| | \"host\" STRING NULL, | +| | \"cpu\" DOUBLE NULL, | +| | \"memory\" DOUBLE NULL, | +| | \"ts\" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX (\"ts\"), | +| | PRIMARY KEY (\"host\") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Created on insertion', | +| | append_mode = 'true' | +| | ) | ++-------+---------------------------------------+\ "; assert_eq!(pretty, expected); diff --git a/tests-integration/tests/http.rs b/tests-integration/tests/http.rs index 410d19547fc3..642f3541aa8b 100644 --- a/tests-integration/tests/http.rs +++ b/tests-integration/tests/http.rs @@ -1681,7 +1681,7 @@ pub async fn test_prometheus_remote_special_labels(store_type: StorageType) { expected, ) .await; - let expected = "[[\"idc3_lo_table\",\"CREATE TABLE IF NOT EXISTS \\\"idc3_lo_table\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n on_physical_table = 'f1'\\n)\"]]"; + let expected = "[[\"idc3_lo_table\",\"CREATE TABLE IF NOT EXISTS \\\"idc3_lo_table\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n \'comment\' = 'Created on insertion',\\n on_physical_table = 'f1'\\n)\"]]"; validate_data( "test_prometheus_remote_special_labels_idc3_show_create_table", &client, @@ -1707,7 +1707,7 @@ pub async fn test_prometheus_remote_special_labels(store_type: StorageType) { expected, ) .await; - let expected = "[[\"idc4_local_table\",\"CREATE TABLE IF NOT EXISTS \\\"idc4_local_table\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n on_physical_table = 'f2'\\n)\"]]"; + let expected = "[[\"idc4_local_table\",\"CREATE TABLE IF NOT EXISTS \\\"idc4_local_table\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n \'comment\' = 'Created on insertion',\\n on_physical_table = 'f2'\\n)\"]]"; validate_data( "test_prometheus_remote_special_labels_idc4_show_create_table", &client, @@ -2169,7 +2169,7 @@ transform: assert_eq!(res.status(), StatusCode::OK); // 3. check schema - let expected_schema = "[[\"logs1\",\"CREATE TABLE IF NOT EXISTS \\\"logs1\\\" (\\n \\\"id1\\\" INT NULL INVERTED INDEX,\\n \\\"id2\\\" INT NULL INVERTED INDEX,\\n \\\"logger\\\" STRING NULL,\\n \\\"type\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"log\\\" STRING NULL FULLTEXT INDEX WITH(analyzer = 'English', backend = 'bloom', case_sensitive = 'false', false_positive_rate = '0.01', granularity = '10240'),\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n TIME INDEX (\\\"time\\\"),\\n PRIMARY KEY (\\\"type\\\", \\\"log\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]"; + let expected_schema = "[[\"logs1\",\"CREATE TABLE IF NOT EXISTS \\\"logs1\\\" (\\n \\\"id1\\\" INT NULL INVERTED INDEX,\\n \\\"id2\\\" INT NULL INVERTED INDEX,\\n \\\"logger\\\" STRING NULL,\\n \\\"type\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"log\\\" STRING NULL FULLTEXT INDEX WITH(analyzer = 'English', backend = 'bloom', case_sensitive = 'false', false_positive_rate = '0.01', granularity = '10240'),\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n TIME INDEX (\\\"time\\\"),\\n PRIMARY KEY (\\\"type\\\", \\\"log\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n \'comment\' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]"; validate_data( "pipeline_schema", &client, @@ -3010,9 +3010,10 @@ table_suffix: _${type} // ) // ENGINE=mito // WITH( + // 'comment' = 'Created on insertion', // append_mode = 'true' // ) - let expected = "[[\"d_table_db\",\"CREATE TABLE IF NOT EXISTS \\\"d_table_db\\\" (\\n \\\"id1_root\\\" INT NULL,\\n \\\"id2_root\\\" INT NULL,\\n \\\"type\\\" STRING NULL,\\n \\\"log\\\" STRING NULL,\\n \\\"logger\\\" STRING NULL,\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n TIME INDEX (\\\"time\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]"; + let expected = "[[\"d_table_db\",\"CREATE TABLE IF NOT EXISTS \\\"d_table_db\\\" (\\n \\\"id1_root\\\" INT NULL,\\n \\\"id2_root\\\" INT NULL,\\n \\\"type\\\" STRING NULL,\\n \\\"log\\\" STRING NULL,\\n \\\"logger\\\" STRING NULL,\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n TIME INDEX (\\\"time\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n 'comment' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]"; validate_data( "test_pipeline_context_db", @@ -3027,11 +3028,12 @@ table_suffix: _${type} // ) // ENGINE=mito // WITH( + // 'comment' = 'Created on insertion', // append_mode = 'true', // skip_wal = 'true', // ttl = '1day' // ) - let expected = "[[\"d_table_http\",\"CREATE TABLE IF NOT EXISTS \\\"d_table_http\\\" (\\n \\\"id1_root\\\" INT NULL,\\n \\\"id2_root\\\" INT NULL,\\n \\\"type\\\" STRING NULL,\\n \\\"log\\\" STRING NULL,\\n \\\"logger\\\" STRING NULL,\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n TIME INDEX (\\\"time\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true',\\n skip_wal = 'true',\\n ttl = '1day'\\n)\"]]"; + let expected = "[[\"d_table_http\",\"CREATE TABLE IF NOT EXISTS \\\"d_table_http\\\" (\\n \\\"id1_root\\\" INT NULL,\\n \\\"id2_root\\\" INT NULL,\\n \\\"type\\\" STRING NULL,\\n \\\"log\\\" STRING NULL,\\n \\\"logger\\\" STRING NULL,\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n TIME INDEX (\\\"time\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n \'comment\' = 'Created on insertion',\\n append_mode = 'true',\\n skip_wal = 'true',\\n ttl = '1day'\\n)\"]]"; validate_data( "test_pipeline_context_http", &client, @@ -3244,13 +3246,14 @@ transform: // ) // ENGINE=mito // WITH( + // 'comment' = 'Created on insertion', // append_mode = 'true' // ) validate_data( "test_pipeline_2_schema", &client, "show create table d_table", - "[[\"d_table\",\"CREATE TABLE IF NOT EXISTS \\\"d_table\\\" (\\n \\\"id1\\\" INT NULL INVERTED INDEX,\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n \\\"id2\\\" STRING NULL,\\n TIME INDEX (\\\"time\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]", + "[[\"d_table\",\"CREATE TABLE IF NOT EXISTS \\\"d_table\\\" (\\n \\\"id1\\\" INT NULL INVERTED INDEX,\\n \\\"time\\\" TIMESTAMP(9) NOT NULL,\\n \\\"id2\\\" STRING NULL,\\n TIME INDEX (\\\"time\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n 'comment' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]", ) .await; @@ -4213,10 +4216,11 @@ pub async fn test_otlp_metrics_new(store_type: StorageType) { // ) // ENGINE=metric // WITH( + // 'comment' = 'Created on insertion', // on_physical_table = 'greptime_physical_table', // otlp_metric_compat = 'prom' // ) - let expected = "[[\"claude_code_cost_usage_USD_total\",\"CREATE TABLE IF NOT EXISTS \\\"claude_code_cost_usage_USD_total\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n \\\"host_arch\\\" STRING NULL,\\n \\\"job\\\" STRING NULL,\\n \\\"model\\\" STRING NULL,\\n \\\"os_version\\\" STRING NULL,\\n \\\"otel_scope_name\\\" STRING NULL,\\n \\\"otel_scope_schema_url\\\" STRING NULL,\\n \\\"otel_scope_version\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL,\\n \\\"service_version\\\" STRING NULL,\\n \\\"session_id\\\" STRING NULL,\\n \\\"terminal_type\\\" STRING NULL,\\n \\\"user_id\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"host_arch\\\", \\\"job\\\", \\\"model\\\", \\\"os_version\\\", \\\"otel_scope_name\\\", \\\"otel_scope_schema_url\\\", \\\"otel_scope_version\\\", \\\"service_name\\\", \\\"service_version\\\", \\\"session_id\\\", \\\"terminal_type\\\", \\\"user_id\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n on_physical_table = 'greptime_physical_table',\\n otlp_metric_compat = 'prom'\\n)\"]]"; + let expected = "[[\"claude_code_cost_usage_USD_total\",\"CREATE TABLE IF NOT EXISTS \\\"claude_code_cost_usage_USD_total\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n \\\"host_arch\\\" STRING NULL,\\n \\\"job\\\" STRING NULL,\\n \\\"model\\\" STRING NULL,\\n \\\"os_version\\\" STRING NULL,\\n \\\"otel_scope_name\\\" STRING NULL,\\n \\\"otel_scope_schema_url\\\" STRING NULL,\\n \\\"otel_scope_version\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL,\\n \\\"service_version\\\" STRING NULL,\\n \\\"session_id\\\" STRING NULL,\\n \\\"terminal_type\\\" STRING NULL,\\n \\\"user_id\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"host_arch\\\", \\\"job\\\", \\\"model\\\", \\\"os_version\\\", \\\"otel_scope_name\\\", \\\"otel_scope_schema_url\\\", \\\"otel_scope_version\\\", \\\"service_name\\\", \\\"service_version\\\", \\\"session_id\\\", \\\"terminal_type\\\", \\\"user_id\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n \'comment\' = 'Created on insertion',\\n on_physical_table = 'greptime_physical_table',\\n otlp_metric_compat = 'prom'\\n)\"]]"; validate_data( "otlp_metrics_all_show_create_table", &client, @@ -4285,10 +4289,11 @@ pub async fn test_otlp_metrics_new(store_type: StorageType) { // ) // ENGINE=metric // WITH( + // 'comment' = 'Created on insertion', // on_physical_table = 'greptime_physical_table', // otlp_metric_compat = 'prom' // ) - let expected = "[[\"claude_code_cost_usage_USD_total\",\"CREATE TABLE IF NOT EXISTS \\\"claude_code_cost_usage_USD_total\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n \\\"job\\\" STRING NULL,\\n \\\"model\\\" STRING NULL,\\n \\\"os_type\\\" STRING NULL,\\n \\\"os_version\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL,\\n \\\"service_version\\\" STRING NULL,\\n \\\"session_id\\\" STRING NULL,\\n \\\"terminal_type\\\" STRING NULL,\\n \\\"user_id\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"job\\\", \\\"model\\\", \\\"os_type\\\", \\\"os_version\\\", \\\"service_name\\\", \\\"service_version\\\", \\\"session_id\\\", \\\"terminal_type\\\", \\\"user_id\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n on_physical_table = 'greptime_physical_table',\\n otlp_metric_compat = 'prom'\\n)\"]]"; + let expected = "[[\"claude_code_cost_usage_USD_total\",\"CREATE TABLE IF NOT EXISTS \\\"claude_code_cost_usage_USD_total\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n \\\"job\\\" STRING NULL,\\n \\\"model\\\" STRING NULL,\\n \\\"os_type\\\" STRING NULL,\\n \\\"os_version\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL,\\n \\\"service_version\\\" STRING NULL,\\n \\\"session_id\\\" STRING NULL,\\n \\\"terminal_type\\\" STRING NULL,\\n \\\"user_id\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"job\\\", \\\"model\\\", \\\"os_type\\\", \\\"os_version\\\", \\\"service_name\\\", \\\"service_version\\\", \\\"session_id\\\", \\\"terminal_type\\\", \\\"user_id\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n 'comment' = 'Created on insertion',\\n on_physical_table = 'greptime_physical_table',\\n otlp_metric_compat = 'prom'\\n)\"]]"; validate_data( "otlp_metrics_show_create_table", &client, @@ -4348,10 +4353,11 @@ pub async fn test_otlp_metrics_new(store_type: StorageType) { // ) // ENGINE=metric // WITH( + // 'comment' = 'Created on insertion', // on_physical_table = 'greptime_physical_table', // otlp_metric_compat = 'prom' // ) - let expected = "[[\"claude_code_cost_usage_USD_total\",\"CREATE TABLE IF NOT EXISTS \\\"claude_code_cost_usage_USD_total\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n \\\"job\\\" STRING NULL,\\n \\\"model\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL,\\n \\\"service_version\\\" STRING NULL,\\n \\\"session_id\\\" STRING NULL,\\n \\\"terminal_type\\\" STRING NULL,\\n \\\"user_id\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"job\\\", \\\"model\\\", \\\"service_name\\\", \\\"service_version\\\", \\\"session_id\\\", \\\"terminal_type\\\", \\\"user_id\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n on_physical_table = 'greptime_physical_table',\\n otlp_metric_compat = 'prom'\\n)\"]]"; + let expected = "[[\"claude_code_cost_usage_USD_total\",\"CREATE TABLE IF NOT EXISTS \\\"claude_code_cost_usage_USD_total\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"greptime_value\\\" DOUBLE NULL,\\n \\\"job\\\" STRING NULL,\\n \\\"model\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL,\\n \\\"service_version\\\" STRING NULL,\\n \\\"session_id\\\" STRING NULL,\\n \\\"terminal_type\\\" STRING NULL,\\n \\\"user_id\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"job\\\", \\\"model\\\", \\\"service_name\\\", \\\"service_version\\\", \\\"session_id\\\", \\\"terminal_type\\\", \\\"user_id\\\")\\n)\\n\\nENGINE=metric\\nWITH(\\n 'comment' = 'Created on insertion',\\n on_physical_table = 'greptime_physical_table',\\n otlp_metric_compat = 'prom'\\n)\"]]"; validate_data( "otlp_metrics_show_create_table_none", &client, @@ -4551,7 +4557,7 @@ pub async fn test_otlp_traces_v1(store_type: StorageType) { let expected = r#"[[1736480942444376000,1736480942444499000,123000,null,"c05d7a4ec8e1f231f02ed6e8da8655b4","d24f921c75f68e23","SPAN_KIND_CLIENT","lets-go","STATUS_CODE_UNSET","","","telemetrygen","","telemetrygen","1.2.3.4","telemetrygen-server",[],[]],[1736480942444376000,1736480942444499000,123000,"d24f921c75f68e23","c05d7a4ec8e1f231f02ed6e8da8655b4","9630f2916e2f7909","SPAN_KIND_SERVER","okey-dokey-0","STATUS_CODE_UNSET","","","telemetrygen","","telemetrygen","1.2.3.4","telemetrygen-client",[],[]],[1736480942444589000,1736480942444712000,123000,null,"cc9e0991a2e63d274984bd44ee669203","eba7be77e3558179","SPAN_KIND_CLIENT","lets-go","STATUS_CODE_UNSET","","","telemetrygen","","telemetrygen","1.2.3.4","telemetrygen-server",[],[]],[1736480942444589000,1736480942444712000,123000,"eba7be77e3558179","cc9e0991a2e63d274984bd44ee669203","8f847259b0f6e1ab","SPAN_KIND_SERVER","okey-dokey-0","STATUS_CODE_UNSET","","","telemetrygen","","telemetrygen","1.2.3.4","telemetrygen-client",[],[]]]"#; validate_data("otlp_traces", &client, "select * from mytable;", expected).await; - let expected_ddl = r#"[["mytable","CREATE TABLE IF NOT EXISTS \"mytable\" (\n \"timestamp\" TIMESTAMP(9) NOT NULL,\n \"timestamp_end\" TIMESTAMP(9) NULL,\n \"duration_nano\" BIGINT UNSIGNED NULL,\n \"parent_span_id\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\n \"trace_id\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\n \"span_id\" STRING NULL,\n \"span_kind\" STRING NULL,\n \"span_name\" STRING NULL,\n \"span_status_code\" STRING NULL,\n \"span_status_message\" STRING NULL,\n \"trace_state\" STRING NULL,\n \"scope_name\" STRING NULL,\n \"scope_version\" STRING NULL,\n \"service_name\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\n \"span_attributes.net.peer.ip\" STRING NULL,\n \"span_attributes.peer.service\" STRING NULL,\n \"span_events\" JSON NULL,\n \"span_links\" JSON NULL,\n TIME INDEX (\"timestamp\"),\n PRIMARY KEY (\"service_name\")\n)\nPARTITION ON COLUMNS (\"trace_id\") (\n trace_id < '1',\n trace_id >= '1' AND trace_id < '2',\n trace_id >= '2' AND trace_id < '3',\n trace_id >= '3' AND trace_id < '4',\n trace_id >= '4' AND trace_id < '5',\n trace_id >= '5' AND trace_id < '6',\n trace_id >= '6' AND trace_id < '7',\n trace_id >= '7' AND trace_id < '8',\n trace_id >= '8' AND trace_id < '9',\n trace_id >= '9' AND trace_id < 'a',\n trace_id >= 'a' AND trace_id < 'b',\n trace_id >= 'b' AND trace_id < 'c',\n trace_id >= 'c' AND trace_id < 'd',\n trace_id >= 'd' AND trace_id < 'e',\n trace_id >= 'e' AND trace_id < 'f',\n trace_id >= 'f'\n)\nENGINE=mito\nWITH(\n append_mode = 'true',\n table_data_model = 'greptime_trace_v1'\n)"]]"#; + let expected_ddl = r#"[["mytable","CREATE TABLE IF NOT EXISTS \"mytable\" (\n \"timestamp\" TIMESTAMP(9) NOT NULL,\n \"timestamp_end\" TIMESTAMP(9) NULL,\n \"duration_nano\" BIGINT UNSIGNED NULL,\n \"parent_span_id\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\n \"trace_id\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\n \"span_id\" STRING NULL,\n \"span_kind\" STRING NULL,\n \"span_name\" STRING NULL,\n \"span_status_code\" STRING NULL,\n \"span_status_message\" STRING NULL,\n \"trace_state\" STRING NULL,\n \"scope_name\" STRING NULL,\n \"scope_version\" STRING NULL,\n \"service_name\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\n \"span_attributes.net.peer.ip\" STRING NULL,\n \"span_attributes.peer.service\" STRING NULL,\n \"span_events\" JSON NULL,\n \"span_links\" JSON NULL,\n TIME INDEX (\"timestamp\"),\n PRIMARY KEY (\"service_name\")\n)\nPARTITION ON COLUMNS (\"trace_id\") (\n trace_id < '1',\n trace_id >= '1' AND trace_id < '2',\n trace_id >= '2' AND trace_id < '3',\n trace_id >= '3' AND trace_id < '4',\n trace_id >= '4' AND trace_id < '5',\n trace_id >= '5' AND trace_id < '6',\n trace_id >= '6' AND trace_id < '7',\n trace_id >= '7' AND trace_id < '8',\n trace_id >= '8' AND trace_id < '9',\n trace_id >= '9' AND trace_id < 'a',\n trace_id >= 'a' AND trace_id < 'b',\n trace_id >= 'b' AND trace_id < 'c',\n trace_id >= 'c' AND trace_id < 'd',\n trace_id >= 'd' AND trace_id < 'e',\n trace_id >= 'e' AND trace_id < 'f',\n trace_id >= 'f'\n)\nENGINE=mito\nWITH(\n 'comment' = 'Created on insertion',\n append_mode = 'true',\n table_data_model = 'greptime_trace_v1'\n)"]]"#; validate_data( "otlp_traces", &client, @@ -4560,7 +4566,7 @@ pub async fn test_otlp_traces_v1(store_type: StorageType) { ) .await; - let expected_ddl = r#"[["mytable_services","CREATE TABLE IF NOT EXISTS \"mytable_services\" (\n \"timestamp\" TIMESTAMP(9) NOT NULL,\n \"service_name\" STRING NULL,\n TIME INDEX (\"timestamp\"),\n PRIMARY KEY (\"service_name\")\n)\n\nENGINE=mito\nWITH(\n append_mode = 'false'\n)"]]"#; + let expected_ddl = r#"[["mytable_services","CREATE TABLE IF NOT EXISTS \"mytable_services\" (\n \"timestamp\" TIMESTAMP(9) NOT NULL,\n \"service_name\" STRING NULL,\n TIME INDEX (\"timestamp\"),\n PRIMARY KEY (\"service_name\")\n)\n\nENGINE=mito\nWITH(\n 'comment' = 'Created on insertion',\n append_mode = 'false'\n)"]]"#; validate_data( "otlp_traces", &client, @@ -4803,7 +4809,7 @@ pub async fn test_loki_pb_logs(store_type: StorageType) { assert_eq!(StatusCode::OK, res.status()); // test schema - let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"line\\\" STRING NULL,\\n \\\"structured_metadata\\\" JSON NULL,\\n \\\"service\\\" STRING NULL,\\n \\\"source\\\" STRING NULL,\\n \\\"wadaxi\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"service\\\", \\\"source\\\", \\\"wadaxi\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]"; + let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"line\\\" STRING NULL,\\n \\\"structured_metadata\\\" JSON NULL,\\n \\\"service\\\" STRING NULL,\\n \\\"source\\\" STRING NULL,\\n \\\"wadaxi\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"service\\\", \\\"source\\\", \\\"wadaxi\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n \'comment\' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]"; validate_data( "loki_pb_schema", &client, @@ -4935,9 +4941,10 @@ processors: // ) // ENGINE=mito // WITH( + // 'comment' = 'Created on insertion', // append_mode = 'true' // ) - let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"loki_label_service\\\" STRING NULL,\\n \\\"loki_label_source\\\" STRING NULL,\\n \\\"loki_label_wadaxi\\\" STRING NULL,\\n \\\"loki_line\\\" STRING NULL,\\n \\\"loki_metadata_key1\\\" STRING NULL,\\n \\\"loki_metadata_key2\\\" STRING NULL,\\n \\\"loki_metadata_key3\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]"; + let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"loki_label_service\\\" STRING NULL,\\n \\\"loki_label_source\\\" STRING NULL,\\n \\\"loki_label_wadaxi\\\" STRING NULL,\\n \\\"loki_line\\\" STRING NULL,\\n \\\"loki_metadata_key1\\\" STRING NULL,\\n \\\"loki_metadata_key2\\\" STRING NULL,\\n \\\"loki_metadata_key3\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n 'comment' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]"; validate_data( "loki_pb_schema", &client, @@ -5007,7 +5014,7 @@ pub async fn test_loki_json_logs(store_type: StorageType) { assert_eq!(StatusCode::OK, res.status()); // test schema - let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"line\\\" STRING NULL,\\n \\\"structured_metadata\\\" JSON NULL,\\n \\\"sender\\\" STRING NULL,\\n \\\"source\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"sender\\\", \\\"source\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]"; + let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"line\\\" STRING NULL,\\n \\\"structured_metadata\\\" JSON NULL,\\n \\\"sender\\\" STRING NULL,\\n \\\"source\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\"),\\n PRIMARY KEY (\\\"sender\\\", \\\"source\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n \'comment\' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]"; validate_data( "loki_json_schema", &client, @@ -5108,9 +5115,10 @@ processors: // ) // ENGINE=mito // WITH( + // 'comment' = 'Created on insertion', // append_mode = 'true' // ) - let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"loki_label_sender\\\" STRING NULL,\\n \\\"loki_label_source\\\" STRING NULL,\\n \\\"loki_line\\\" STRING NULL,\\n \\\"loki_metadata_key1\\\" STRING NULL,\\n \\\"loki_metadata_key2\\\" STRING NULL,\\n \\\"loki_metadata_key3\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'true'\\n)\"]]"; + let expected = "[[\"loki_table_name\",\"CREATE TABLE IF NOT EXISTS \\\"loki_table_name\\\" (\\n \\\"greptime_timestamp\\\" TIMESTAMP(3) NOT NULL,\\n \\\"loki_label_sender\\\" STRING NULL,\\n \\\"loki_label_source\\\" STRING NULL,\\n \\\"loki_line\\\" STRING NULL,\\n \\\"loki_metadata_key1\\\" STRING NULL,\\n \\\"loki_metadata_key2\\\" STRING NULL,\\n \\\"loki_metadata_key3\\\" STRING NULL,\\n TIME INDEX (\\\"greptime_timestamp\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n 'comment' = 'Created on insertion',\\n append_mode = 'true'\\n)\"]]"; validate_data( "loki_json_schema", &client, diff --git a/tests/cases/distributed/flow-tql/flow_tql.result b/tests/cases/distributed/flow-tql/flow_tql.result index 6afffc6edb7e..b1e62b3317d2 100644 --- a/tests/cases/distributed/flow-tql/flow_tql.result +++ b/tests/cases/distributed/flow-tql/flow_tql.result @@ -15,20 +15,22 @@ Affected Rows: 0 SHOW CREATE TABLE cnt_reqs; -+----------+-------------------------------------------+ -| Table | Create Table | -+----------+-------------------------------------------+ -| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | -| | "count(http_requests.val)" DOUBLE NULL, | -| | "ts" TIMESTAMP(3) NOT NULL, | -| | "status_code" STRING NULL, | -| | TIME INDEX ("ts"), | -| | PRIMARY KEY ("status_code") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+----------+-------------------------------------------+ ++----------+---------------------------------------------------+ +| Table | Create Table | ++----------+---------------------------------------------------+ +| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | +| | "count(http_requests.val)" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "status_code" STRING NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("status_code") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++----------+---------------------------------------------------+ -- test if sink table is tql queryable TQL EVAL (now() - '1m'::interval, now(), '5s') count_values("status_code", cnt_reqs); @@ -157,20 +159,22 @@ Affected Rows: 0 SHOW CREATE TABLE cnt_reqs; -+----------+-------------------------------------------+ -| Table | Create Table | -+----------+-------------------------------------------+ -| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | -| | "count(http_requests.val)" DOUBLE NULL, | -| | "ts" TIMESTAMP(3) NOT NULL, | -| | "status_code" STRING NULL, | -| | TIME INDEX ("ts"), | -| | PRIMARY KEY ("status_code") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+----------+-------------------------------------------+ ++----------+---------------------------------------------------+ +| Table | Create Table | ++----------+---------------------------------------------------+ +| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | +| | "count(http_requests.val)" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "status_code" STRING NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("status_code") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++----------+---------------------------------------------------+ -- test if sink table is tql queryable TQL EVAL (now() - '1m'::interval, now(), '5s') count_values("status_code", cnt_reqs); @@ -258,7 +262,9 @@ SHOW CREATE TABLE rate_reqs; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +-----------+-----------------------------------------------------------+ -- test if sink table is tql queryable @@ -337,7 +343,9 @@ SHOW CREATE TABLE rate_reqs; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +-----------+-----------------------------------------------------------+ -- test if sink table is tql queryable diff --git a/tests/cases/standalone/common/comment.result b/tests/cases/standalone/common/comment.result index b5e186f48304..19f9b8776b9b 100644 --- a/tests/cases/standalone/common/comment.result +++ b/tests/cases/standalone/common/comment.result @@ -27,7 +27,9 @@ SHOW CREATE TABLE comment_table_test; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | comment = 'table level description' | +| | ) | +--------------------+---------------------------------------------------+ -- Remove table comment @@ -115,8 +117,8 @@ DROP TABLE comment_column_test; Affected Rows: 0 -- Test: COMMENT ON FLOW add & remove --- Prepare base & sink tables -CREATE TABLE flow_base_comment_test ( +-- Prepare source & sink tables +CREATE TABLE flow_source_comment_test ( desc_str STRING, ts TIMESTAMP TIME INDEX ); @@ -133,7 +135,7 @@ Affected Rows: 0 CREATE FLOW flow_comment_test SINK TO flow_sink_comment_test AS -SELECT desc_str, ts FROM flow_base_comment_test; +SELECT desc_str, ts FROM flow_source_comment_test; Affected Rows: 0 @@ -144,14 +146,14 @@ Affected Rows: 0 SHOW CREATE FLOW flow_comment_test; -+-------------------+----------------------------------------------------+ -| Flow | Create Flow | -+-------------------+----------------------------------------------------+ -| flow_comment_test | CREATE FLOW IF NOT EXISTS flow_comment_test | -| | SINK TO flow_sink_comment_test | -| | COMMENT 'flow level description' | -| | AS SELECT desc_str, ts FROM flow_base_comment_test | -+-------------------+----------------------------------------------------+ ++-------------------+------------------------------------------------------+ +| Flow | Create Flow | ++-------------------+------------------------------------------------------+ +| flow_comment_test | CREATE FLOW IF NOT EXISTS flow_comment_test | +| | SINK TO flow_sink_comment_test | +| | COMMENT 'flow level description' | +| | AS SELECT desc_str, ts FROM flow_source_comment_test | ++-------------------+------------------------------------------------------+ -- Remove flow comment COMMENT ON FLOW flow_comment_test IS NULL; @@ -160,19 +162,19 @@ Affected Rows: 0 SHOW CREATE FLOW flow_comment_test; -+-------------------+----------------------------------------------------+ -| Flow | Create Flow | -+-------------------+----------------------------------------------------+ -| flow_comment_test | CREATE FLOW IF NOT EXISTS flow_comment_test | -| | SINK TO flow_sink_comment_test | -| | AS SELECT desc_str, ts FROM flow_base_comment_test | -+-------------------+----------------------------------------------------+ ++-------------------+------------------------------------------------------+ +| Flow | Create Flow | ++-------------------+------------------------------------------------------+ +| flow_comment_test | CREATE FLOW IF NOT EXISTS flow_comment_test | +| | SINK TO flow_sink_comment_test | +| | AS SELECT desc_str, ts FROM flow_source_comment_test | ++-------------------+------------------------------------------------------+ DROP FLOW flow_comment_test; Affected Rows: 0 -DROP TABLE flow_base_comment_test; +DROP TABLE flow_source_comment_test; Affected Rows: 0 diff --git a/tests/cases/standalone/common/flow/flow_advance_ttl.result b/tests/cases/standalone/common/flow/flow_advance_ttl.result index 05ae665be86c..12b27ace13d8 100644 --- a/tests/cases/standalone/common/flow/flow_advance_ttl.result +++ b/tests/cases/standalone/common/flow/flow_advance_ttl.result @@ -46,6 +46,7 @@ SHOW CREATE TABLE distinct_basic; | | ) | +----------------+-----------------------------------------------------------+ +-- SQLNESS REPLACE \d{4} REDACTED SHOW CREATE TABLE out_distinct_basic; +--------------------+---------------------------------------------------+ @@ -60,7 +61,9 @@ SHOW CREATE TABLE out_distinct_basic; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Sink table for flow flow-id=REDACTED' | +| | ) | +--------------------+---------------------------------------------------+ -- SQLNESS SLEEP 3s @@ -242,7 +245,9 @@ SHOW CREATE TABLE out_distinct_basic; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +--------------------+---------------------------------------------------+ SELECT diff --git a/tests/cases/standalone/common/flow/flow_advance_ttl.sql b/tests/cases/standalone/common/flow/flow_advance_ttl.sql index 141c595e8958..9574eabd918c 100644 --- a/tests/cases/standalone/common/flow/flow_advance_ttl.sql +++ b/tests/cases/standalone/common/flow/flow_advance_ttl.sql @@ -20,6 +20,7 @@ SELECT flow_name, options FROM INFORMATION_SCHEMA.FLOWS; SHOW CREATE TABLE distinct_basic; +-- SQLNESS REPLACE \d{4} REDACTED SHOW CREATE TABLE out_distinct_basic; -- SQLNESS SLEEP 3s diff --git a/tests/cases/standalone/common/flow/flow_auto_sink_table.result b/tests/cases/standalone/common/flow/flow_auto_sink_table.result index f1d229e6e8c7..90d53b9598ac 100644 --- a/tests/cases/standalone/common/flow/flow_auto_sink_table.result +++ b/tests/cases/standalone/common/flow/flow_auto_sink_table.result @@ -20,19 +20,21 @@ Affected Rows: 0 SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "sum(numbers_input_basic.number)" BIGINT NULL, | -| | "time_window" TIMESTAMP(9) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("time_window") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "sum(numbers_input_basic.number)" BIGINT NULL, | +| | "time_window" TIMESTAMP(9) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("time_window") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ -- SQLNESS REPLACE (ADMIN\sFLUSH_FLOW\('\w+'\)\s+\|\n\+-+\+\n\|\s+)[0-9]+\s+\| $1 FLOW_FLUSHED | ADMIN FLUSH_FLOW('test_numbers_basic'); @@ -55,19 +57,21 @@ SELECT 1; -- SQLNESS SLEEP 3s SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "sum(numbers_input_basic.number)" BIGINT NULL, | -| | "time_window" TIMESTAMP(9) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("time_window") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "sum(numbers_input_basic.number)" BIGINT NULL, | +| | "time_window" TIMESTAMP(9) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("time_window") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ SHOW CREATE FLOW test_numbers_basic; @@ -122,19 +126,21 @@ SELECT 1; -- SQLNESS SLEEP 3s SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "sumup" BIGINT NULL, | -| | "event_time" TIMESTAMP(3) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("event_time") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "sumup" BIGINT NULL, | +| | "event_time" TIMESTAMP(3) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("event_time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ -- SQLNESS ARG restart=true SELECT 1; @@ -158,19 +164,21 @@ SHOW CREATE FLOW test_numbers_basic; SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "sumup" BIGINT NULL, | -| | "event_time" TIMESTAMP(3) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("event_time") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "sumup" BIGINT NULL, | +| | "event_time" TIMESTAMP(3) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("event_time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ DROP FLOW test_numbers_basic; diff --git a/tests/cases/standalone/common/flow/flow_basic.result b/tests/cases/standalone/common/flow/flow_basic.result index e089af178127..ef03d16b99c5 100644 --- a/tests/cases/standalone/common/flow/flow_basic.result +++ b/tests/cases/standalone/common/flow/flow_basic.result @@ -20,19 +20,21 @@ Affected Rows: 0 SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "sum(numbers_input_basic.number)" BIGINT NULL, | -| | "time_window" TIMESTAMP(9) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("time_window") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "sum(numbers_input_basic.number)" BIGINT NULL, | +| | "time_window" TIMESTAMP(9) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("time_window") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ -- TODO(discord9): confirm if it's necessary to flush flow here? -- because flush_flow result is at most 1 @@ -47,19 +49,21 @@ ADMIN FLUSH_FLOW('test_numbers_basic'); SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "sum(numbers_input_basic.number)" BIGINT NULL, | -| | "time_window" TIMESTAMP(9) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("time_window") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "sum(numbers_input_basic.number)" BIGINT NULL, | +| | "time_window" TIMESTAMP(9) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("time_window") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ -- SQLNESS ARG restart=true SELECT 1; @@ -172,19 +176,21 @@ Affected Rows: 0 SHOW CREATE TABLE out_basic; -+-----------+---------------------------------------------+ -| Table | Create Table | -+-----------+---------------------------------------------+ -| out_basic | CREATE TABLE IF NOT EXISTS "out_basic" ( | -| | "wildcard" BIGINT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("__ts_placeholder") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-----------+---------------------------------------------+ ++-----------+---------------------------------------------------+ +| Table | Create Table | ++-----------+---------------------------------------------------+ +| out_basic | CREATE TABLE IF NOT EXISTS "out_basic" ( | +| | "wildcard" BIGINT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("__ts_placeholder") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-----------+---------------------------------------------------+ DROP FLOW test_wildcard_basic; @@ -200,19 +206,21 @@ Affected Rows: 0 SHOW CREATE TABLE out_basic; -+-----------+---------------------------------------------+ -| Table | Create Table | -+-----------+---------------------------------------------+ -| out_basic | CREATE TABLE IF NOT EXISTS "out_basic" ( | -| | "wildcard" BIGINT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("__ts_placeholder") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-----------+---------------------------------------------+ ++-----------+---------------------------------------------------+ +| Table | Create Table | ++-----------+---------------------------------------------------+ +| out_basic | CREATE TABLE IF NOT EXISTS "out_basic" ( | +| | "wildcard" BIGINT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("__ts_placeholder") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-----------+---------------------------------------------------+ -- SQLNESS ARG restart=true SELECT 1; @@ -243,19 +251,21 @@ ADMIN FLUSH_FLOW('test_wildcard_basic'); SHOW CREATE TABLE out_basic; -+-----------+---------------------------------------------+ -| Table | Create Table | -+-----------+---------------------------------------------+ -| out_basic | CREATE TABLE IF NOT EXISTS "out_basic" ( | -| | "wildcard" BIGINT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("__ts_placeholder") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-----------+---------------------------------------------+ ++-----------+---------------------------------------------------+ +| Table | Create Table | ++-----------+---------------------------------------------------+ +| out_basic | CREATE TABLE IF NOT EXISTS "out_basic" ( | +| | "wildcard" BIGINT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("__ts_placeholder") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-----------+---------------------------------------------------+ SELECT wildcard FROM out_basic; @@ -309,7 +319,9 @@ SHOW CREATE TABLE out_distinct_basic; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +--------------------+---------------------------------------------------+ -- TODO(discord9): confirm if it's necessary to flush flow here? @@ -365,7 +377,9 @@ SHOW CREATE TABLE out_distinct_basic; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +--------------------+---------------------------------------------------+ SELECT @@ -637,20 +651,22 @@ Affected Rows: 0 SHOW CREATE TABLE ngx_country; -+-------------+---------------------------------------------+ -| Table | Create Table | -+-------------+---------------------------------------------+ -| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | -| | "country" STRING NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("__ts_placeholder"), | -| | PRIMARY KEY ("country") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------+---------------------------------------------+ ++-------------+---------------------------------------------------+ +| Table | Create Table | ++-------------+---------------------------------------------------+ +| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | +| | "country" STRING NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("__ts_placeholder"), | +| | PRIMARY KEY ("country") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------+---------------------------------------------------+ INSERT INTO ngx_access_log @@ -670,20 +686,22 @@ ADMIN FLUSH_FLOW('calc_ngx_country'); SHOW CREATE TABLE ngx_country; -+-------------+---------------------------------------------+ -| Table | Create Table | -+-------------+---------------------------------------------+ -| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | -| | "country" STRING NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("__ts_placeholder"), | -| | PRIMARY KEY ("country") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------+---------------------------------------------+ ++-------------+---------------------------------------------------+ +| Table | Create Table | ++-------------+---------------------------------------------------+ +| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | +| | "country" STRING NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("__ts_placeholder"), | +| | PRIMARY KEY ("country") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------+---------------------------------------------------+ SELECT country @@ -787,20 +805,22 @@ Affected Rows: 0 SHOW CREATE TABLE ngx_country; -+-------------+--------------------------------------------+ -| Table | Create Table | -+-------------+--------------------------------------------+ -| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | -| | "country" STRING NULL, | -| | "time_window" TIMESTAMP(3) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("time_window"), | -| | PRIMARY KEY ("country") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------+--------------------------------------------+ ++-------------+---------------------------------------------------+ +| Table | Create Table | ++-------------+---------------------------------------------------+ +| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | +| | "country" STRING NULL, | +| | "time_window" TIMESTAMP(3) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("time_window"), | +| | PRIMARY KEY ("country") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------+---------------------------------------------------+ INSERT INTO ngx_access_log @@ -820,20 +840,22 @@ ADMIN FLUSH_FLOW('calc_ngx_country'); SHOW CREATE TABLE ngx_country; -+-------------+--------------------------------------------+ -| Table | Create Table | -+-------------+--------------------------------------------+ -| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | -| | "country" STRING NULL, | -| | "time_window" TIMESTAMP(3) NOT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | TIME INDEX ("time_window"), | -| | PRIMARY KEY ("country") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------+--------------------------------------------+ ++-------------+---------------------------------------------------+ +| Table | Create Table | ++-------------+---------------------------------------------------+ +| ngx_country | CREATE TABLE IF NOT EXISTS "ngx_country" ( | +| | "country" STRING NULL, | +| | "time_window" TIMESTAMP(3) NOT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | TIME INDEX ("time_window"), | +| | PRIMARY KEY ("country") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------+---------------------------------------------------+ SELECT country, @@ -1673,19 +1695,21 @@ Affected Rows: 0 SHOW CREATE TABLE out_num_cnt_basic; -+-------------------+--------------------------------------------------+ -| Table | Create Table | -+-------------------+--------------------------------------------------+ -| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | -| | "avg_after_filter_num" BIGINT NULL, | -| | "update_at" TIMESTAMP(3) NULL, | -| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | -| | TIME INDEX ("__ts_placeholder") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+-------------------+--------------------------------------------------+ ++-------------------+---------------------------------------------------+ +| Table | Create Table | ++-------------------+---------------------------------------------------+ +| out_num_cnt_basic | CREATE TABLE IF NOT EXISTS "out_num_cnt_basic" ( | +| | "avg_after_filter_num" BIGINT NULL, | +| | "update_at" TIMESTAMP(3) NULL, | +| | "__ts_placeholder" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("__ts_placeholder") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++-------------------+---------------------------------------------------+ -- TODO(discord9): confirm if it's necessary to flush flow here? -- because flush_flow result is at most 1 diff --git a/tests/cases/standalone/flow-tql/flow_tql.result b/tests/cases/standalone/flow-tql/flow_tql.result index 6fb9386e83b5..1661eacf2484 100644 --- a/tests/cases/standalone/flow-tql/flow_tql.result +++ b/tests/cases/standalone/flow-tql/flow_tql.result @@ -15,20 +15,22 @@ Affected Rows: 0 SHOW CREATE TABLE cnt_reqs; -+----------+-------------------------------------------+ -| Table | Create Table | -+----------+-------------------------------------------+ -| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | -| | "count(http_requests.val)" DOUBLE NULL, | -| | "ts" TIMESTAMP(3) NOT NULL, | -| | "status_code" STRING NULL, | -| | TIME INDEX ("ts"), | -| | PRIMARY KEY ("status_code") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+----------+-------------------------------------------+ ++----------+---------------------------------------------------+ +| Table | Create Table | ++----------+---------------------------------------------------+ +| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | +| | "count(http_requests.val)" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "status_code" STRING NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("status_code") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++----------+---------------------------------------------------+ -- test if sink table is tql queryable TQL EVAL (now() - '1m'::interval, now(), '5s') count_values("status_code", cnt_reqs); @@ -157,20 +159,22 @@ Affected Rows: 0 SHOW CREATE TABLE cnt_reqs; -+----------+-------------------------------------------+ -| Table | Create Table | -+----------+-------------------------------------------+ -| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | -| | "count(http_requests.val)" DOUBLE NULL, | -| | "ts" TIMESTAMP(3) NOT NULL, | -| | "status_code" STRING NULL, | -| | TIME INDEX ("ts"), | -| | PRIMARY KEY ("status_code") | -| | ) | -| | | -| | ENGINE=mito | -| | | -+----------+-------------------------------------------+ ++----------+---------------------------------------------------+ +| Table | Create Table | ++----------+---------------------------------------------------+ +| cnt_reqs | CREATE TABLE IF NOT EXISTS "cnt_reqs" ( | +| | "count(http_requests.val)" DOUBLE NULL, | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "status_code" STRING NULL, | +| | TIME INDEX ("ts"), | +| | PRIMARY KEY ("status_code") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | ++----------+---------------------------------------------------+ -- test if sink table is tql queryable TQL EVAL (now() - '1m'::interval, now(), '5s') count_values("status_code", cnt_reqs); @@ -258,7 +262,9 @@ SHOW CREATE TABLE rate_reqs; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +-----------+-----------------------------------------------------------+ -- test if sink table is tql queryable @@ -337,7 +343,9 @@ SHOW CREATE TABLE rate_reqs; | | ) | | | | | | ENGINE=mito | -| | | +| | WITH( | +| | 'comment' = 'Auto created table by flow engine' | +| | ) | +-----------+-----------------------------------------------------------+ -- test if sink table is tql queryable From 079c34a9f0b9e84f600630421f51bb6db09e581f Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 20:56:12 +0800 Subject: [PATCH 09/21] simplify table info serde Signed-off-by: Ruihang Xia --- src/common/meta/src/key/table_info.rs | 2 +- src/operator/src/statement/comment.rs | 35 +++++--------------------- src/operator/src/statement/show.rs | 4 +-- src/query/src/sql/show_create_table.rs | 8 +++--- 4 files changed, 12 insertions(+), 37 deletions(-) diff --git a/src/common/meta/src/key/table_info.rs b/src/common/meta/src/key/table_info.rs index 42cdbe3dc8f6..8a0e018a56f4 100644 --- a/src/common/meta/src/key/table_info.rs +++ b/src/common/meta/src/key/table_info.rs @@ -94,7 +94,7 @@ impl TableInfoValue { } } - pub(crate) fn update(&self, new_table_info: RawTableInfo) -> Self { + pub fn update(&self, new_table_info: RawTableInfo) -> Self { Self { table_info: new_table_info, version: self.version + 1, diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index e1bd82351b2b..ae294b30ca5e 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -23,11 +23,12 @@ use common_meta::instruction::CacheIdent; use common_meta::key::flow::flow_info::{FlowInfoKey, FlowInfoValue}; use common_meta::key::flow::flow_route::FlowRouteValue; use common_meta::key::table_info::{TableInfoKey, TableInfoValue}; -use common_meta::key::{DeserializedValueWithBytes, FlowId, FlowPartitionId, MetadataKey}; +use common_meta::key::{ + DeserializedValueWithBytes, FlowId, FlowPartitionId, MetadataKey, MetadataValue, +}; use common_meta::rpc::store::PutRequest; use common_query::Output; use datatypes::schema::COMMENT_KEY as COLUMN_COMMENT_KEY; -use serde_json::{Map, Value}; use session::context::QueryContextRef; use session::table_name::table_idents_to_full_name; use snafu::{GenerateImplicitData, Location, OptionExt, ResultExt}; @@ -307,33 +308,9 @@ impl StatementExecutor { ) -> Result<()> { let kv_backend = Arc::clone(self.table_metadata_manager.kv_backend()); let table_id = current_table_info.table_info.ident.table_id; - let convert_err = |err| common_meta::error::Error::SerdeJson { - error: err, - location: Location::generate(), - }; - - let mut stored_obj: Map = - serde_json::from_slice(¤t_table_info.get_raw_bytes()) - .map_err(convert_err) - .context(TableMetadataManagerSnafu)?; - - let current_version = stored_obj - .get("version") - .and_then(Value::as_u64) - .unwrap_or_default(); - stored_obj.insert( - "version".to_string(), - Value::from(current_version.saturating_add(1)), - ); - stored_obj.insert( - "table_info".to_string(), - serde_json::to_value(&new_table_info) - .map_err(convert_err) - .context(TableMetadataManagerSnafu)?, - ); - - let raw_value = serde_json::to_vec(&stored_obj) - .map_err(convert_err) + let new_table_info_value = current_table_info.update(new_table_info); + let raw_value = new_table_info_value + .try_as_raw_value() .context(TableMetadataManagerSnafu)?; kv_backend diff --git a/src/operator/src/statement/show.rs b/src/operator/src/statement/show.rs index 65c6801a0dba..f8acc71cfafc 100644 --- a/src/operator/src/statement/show.rs +++ b/src/operator/src/statement/show.rs @@ -143,9 +143,7 @@ impl StatementExecutor { if !partition_column_names.is_empty() { latest_info.meta.partition_key_indices = partition_column_names .iter() - .filter_map(|name| { - latest_info.meta.schema.column_index_by_name(name.as_str()) - }) + .filter_map(|name| latest_info.meta.schema.column_index_by_name(name.as_str())) .collect(); } diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs index 3efccc0c5a17..bfb7eef248ae 100644 --- a/src/query/src/sql/show_create_table.rs +++ b/src/query/src/sql/show_create_table.rs @@ -241,10 +241,10 @@ pub fn create_table_stmt( let constraints = create_table_constraints(&table_meta.engine, schema, table_meta, quote_style); let mut options = create_sql_options(table_meta, schema_options); - if let Some(comment) = &table_info.desc { - if options.get(TABLE_COMMENT_KEY).is_none() { - options.insert(format!("'{TABLE_COMMENT_KEY}'"), comment.clone()); - } + if let Some(comment) = &table_info.desc + && options.get(TABLE_COMMENT_KEY).is_none() + { + options.insert(format!("'{TABLE_COMMENT_KEY}'"), comment.clone()); } Ok(CreateTable { From 695261edeff13aa1b3bcb5a01113713aeacea463 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 21:52:42 +0800 Subject: [PATCH 10/21] don't txn Signed-off-by: Ruihang Xia --- src/operator/src/statement/comment.rs | 92 +++------------------------ 1 file changed, 10 insertions(+), 82 deletions(-) diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index ae294b30ca5e..58ce538b15e2 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -12,20 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::sync::Arc; - use chrono::Utc; use common_catalog::format_full_table_name; use common_error::ext::BoxedError; use common_meta::cache_invalidator::Context; -use common_meta::error::Error as MetaError; use common_meta::instruction::CacheIdent; use common_meta::key::flow::flow_info::{FlowInfoKey, FlowInfoValue}; -use common_meta::key::flow::flow_route::FlowRouteValue; use common_meta::key::table_info::{TableInfoKey, TableInfoValue}; -use common_meta::key::{ - DeserializedValueWithBytes, FlowId, FlowPartitionId, MetadataKey, MetadataValue, -}; +use common_meta::key::{DeserializedValueWithBytes, FlowId, MetadataKey, MetadataValue}; use common_meta::rpc::store::PutRequest; use common_query::Output; use datatypes::schema::COMMENT_KEY as COLUMN_COMMENT_KEY; @@ -87,8 +81,7 @@ impl StatementExecutor { new_table_info.desc.as_deref(), ); - self.update_table_info_with_fallback(&table_info, new_table_info) - .await?; + self.update_table_info(&table_info, new_table_info).await?; self.invalidate_table_cache_by_name(table_id, table_name) .await?; @@ -128,8 +121,7 @@ impl StatementExecutor { update_column_comment_metadata(column_schema, comment); - self.update_table_info_with_fallback(&table_info, new_table_info) - .await?; + self.update_table_info(&table_info, new_table_info).await?; self.invalidate_table_cache_by_name(table_id, table_name) .await?; @@ -184,18 +176,7 @@ impl StatementExecutor { new_flow_info.comment = comment.unwrap_or_default(); new_flow_info.updated_time = Utc::now(); - let flow_routes = self - .flow_metadata_manager - .flow_route_manager() - .routes(flow_id) - .await - .context(TableMetadataManagerSnafu)? - .into_iter() - .map(|(key, value)| (key.partition_id(), value)) - .collect::>(); - - self.update_flow_metadata_with_fallback(flow_id, &flow_info, new_flow_info, flow_routes) - .await?; + self.update_flow_metadata(flow_id, new_flow_info).await?; Ok(Output::new_with_affected_rows(0)) } @@ -257,63 +238,19 @@ impl StatementExecutor { Ok(()) } - async fn update_table_info_with_fallback( + async fn update_table_info( &self, current_table_info: &DeserializedValueWithBytes, new_table_info: RawTableInfo, ) -> Result<()> { - let try_update = self - .table_metadata_manager - .update_table_info(current_table_info, None, new_table_info.clone()) - .await - .context(TableMetadataManagerSnafu); - - match try_update { - Ok(()) => Ok(()), - Err(err) if Self::is_txn_unsupported(&err) => { - self.update_table_info_without_txn(current_table_info, new_table_info) - .await - } - Err(err) => Err(err), - } - } - - async fn update_flow_metadata_with_fallback( - &self, - flow_id: FlowId, - current_flow_info: &DeserializedValueWithBytes, - new_flow_info: FlowInfoValue, - flow_routes: Vec<(FlowPartitionId, FlowRouteValue)>, - ) -> Result<()> { - let try_update = self - .flow_metadata_manager - .update_flow_metadata(flow_id, current_flow_info, &new_flow_info, flow_routes) - .await - .context(TableMetadataManagerSnafu); - - match try_update { - Ok(()) => Ok(()), - Err(err) if Self::is_txn_unsupported(&err) => { - self.update_flow_metadata_without_txn(flow_id, new_flow_info) - .await - } - Err(err) => Err(err), - } - } - - async fn update_table_info_without_txn( - &self, - current_table_info: &DeserializedValueWithBytes, - new_table_info: RawTableInfo, - ) -> Result<()> { - let kv_backend = Arc::clone(self.table_metadata_manager.kv_backend()); let table_id = current_table_info.table_info.ident.table_id; let new_table_info_value = current_table_info.update(new_table_info); let raw_value = new_table_info_value .try_as_raw_value() .context(TableMetadataManagerSnafu)?; - kv_backend + self.table_metadata_manager + .kv_backend() .put( PutRequest::new() .with_key(TableInfoKey::new(table_id).to_bytes()) @@ -325,12 +262,11 @@ impl StatementExecutor { Ok(()) } - async fn update_flow_metadata_without_txn( + async fn update_flow_metadata( &self, flow_id: FlowId, new_flow_info: FlowInfoValue, ) -> Result<()> { - let kv_backend = Arc::clone(self.table_metadata_manager.kv_backend()); let raw_value = serde_json::to_vec(&new_flow_info) .map_err(|err| common_meta::error::Error::SerdeJson { error: err, @@ -338,7 +274,8 @@ impl StatementExecutor { }) .context(TableMetadataManagerSnafu)?; - kv_backend + self.table_metadata_manager + .kv_backend() .put( PutRequest::new() .with_key(FlowInfoKey::new(flow_id).to_bytes()) @@ -349,15 +286,6 @@ impl StatementExecutor { Ok(()) } - - fn is_txn_unsupported(error: &crate::error::Error) -> bool { - matches!( - error, - crate::error::Error::TableMetadataManager { source, .. } - if matches!(source, MetaError::Unsupported { operation, .. } - if operation == "txn" || operation.ends_with("::txn")) - ) - } } fn update_column_comment_metadata( From eb9d3513d2302481a950001f23db12596dab8c26 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 9 Oct 2025 22:28:20 +0800 Subject: [PATCH 11/21] remove empty trait Signed-off-by: Ruihang Xia --- src/catalog/src/kvbackend/client.rs | 16 +--------------- src/operator/src/statement/show.rs | 10 +++------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/src/catalog/src/kvbackend/client.rs b/src/catalog/src/kvbackend/client.rs index 0768ac5e9dad..f74509217fb0 100644 --- a/src/catalog/src/kvbackend/client.rs +++ b/src/catalog/src/kvbackend/client.rs @@ -21,9 +21,7 @@ use std::time::Duration; use common_error::ext::BoxedError; use common_meta::cache_invalidator::KvCacheInvalidator; use common_meta::error::Error::CacheNotGet; -use common_meta::error::{ - CacheNotGetSnafu, Error, ExternalSnafu, GetKvCacheSnafu, Result, UnsupportedSnafu, -}; +use common_meta::error::{CacheNotGetSnafu, Error, ExternalSnafu, GetKvCacheSnafu, Result}; use common_meta::kv_backend::txn::{Txn, TxnResponse}; use common_meta::kv_backend::{KvBackend, KvBackendRef, TxnService}; use common_meta::rpc::KeyValue; @@ -370,20 +368,8 @@ impl MetaKvBackend { } } -#[async_trait::async_trait] impl TxnService for MetaKvBackend { type Error = Error; - - async fn txn(&self, _txn: Txn) -> std::result::Result { - UnsupportedSnafu { - operation: "MetaKvBackend::txn", - } - .fail() - } - - fn max_txn_ops(&self) -> usize { - 0 - } } /// Implement `KvBackend` trait for `MetaKvBackend` instead of opendal's `Accessor` since diff --git a/src/operator/src/statement/show.rs b/src/operator/src/statement/show.rs index f8acc71cfafc..f1766d8a7d94 100644 --- a/src/operator/src/statement/show.rs +++ b/src/operator/src/statement/show.rs @@ -122,13 +122,9 @@ impl StatementExecutor { table: TableRef, query_ctx: QueryContextRef, ) -> Result { - let table_info_with_partition_indices = table.table_info(); - let partition_column_names: Vec<_> = table_info_with_partition_indices - .meta - .partition_column_names() - .cloned() - .collect(); - let mut table_info = table_info_with_partition_indices; + let mut table_info = table.table_info(); + let partition_column_names: Vec<_> = + table_info.meta.partition_column_names().cloned().collect(); if let Some(latest) = self .table_metadata_manager From 8557dcce94f4e2f430fced71d147e06151dc3635 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Sat, 11 Oct 2025 17:36:11 +0800 Subject: [PATCH 12/21] wip: procedure Signed-off-by: Ruihang Xia --- src/common/meta/src/ddl.rs | 1 + src/common/meta/src/ddl/comment_on.rs | 446 ++++++++++++++++++++++++++ src/common/meta/src/ddl_manager.rs | 50 ++- src/common/meta/src/rpc/ddl.rs | 134 +++++++- src/operator/src/statement/comment.rs | 403 +++++------------------ 5 files changed, 690 insertions(+), 344 deletions(-) create mode 100644 src/common/meta/src/ddl/comment_on.rs diff --git a/src/common/meta/src/ddl.rs b/src/common/meta/src/ddl.rs index e12331d4a26b..7af538f785f3 100644 --- a/src/common/meta/src/ddl.rs +++ b/src/common/meta/src/ddl.rs @@ -31,6 +31,7 @@ use crate::region_registry::LeaderRegionRegistryRef; pub mod alter_database; pub mod alter_logical_tables; pub mod alter_table; +pub mod comment_on; pub mod create_database; pub mod create_flow; pub mod create_logical_tables; diff --git a/src/common/meta/src/ddl/comment_on.rs b/src/common/meta/src/ddl/comment_on.rs new file mode 100644 index 000000000000..f5edd0504df5 --- /dev/null +++ b/src/common/meta/src/ddl/comment_on.rs @@ -0,0 +1,446 @@ +// Copyright 2023 Greptime Team +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use async_trait::async_trait; +use chrono::Utc; +use common_catalog::format_full_table_name; +use common_procedure::error::{FromJsonSnafu, Result as ProcedureResult, ToJsonSnafu}; +use common_procedure::{Context as ProcedureContext, LockKey, Procedure, Status}; +use common_telemetry::tracing::info; +use datatypes::schema::COMMENT_KEY as COLUMN_COMMENT_KEY; +use serde::{Deserialize, Serialize}; +use snafu::{OptionExt, ResultExt, ensure}; +use store_api::storage::TableId; +use strum::AsRefStr; +use table::metadata::RawTableInfo; +use table::requests::COMMENT_KEY as TABLE_COMMENT_KEY; +use table::table_name::TableName; + +use crate::cache_invalidator::Context; +use crate::ddl::DdlContext; +use crate::ddl::utils::map_to_procedure_error; +use crate::error::{FlowNotFoundSnafu, Result, TableNotFoundSnafu, UnexpectedSnafu}; +use crate::instruction::CacheIdent; +use crate::key::flow::flow_info::{FlowInfoKey, FlowInfoValue}; +use crate::key::table_info::{TableInfoKey, TableInfoValue}; +use crate::key::table_name::TableNameKey; +use crate::key::{DeserializedValueWithBytes, FlowId, MetadataKey, MetadataValue}; +use crate::lock_key::{CatalogLock, FlowNameLock, SchemaLock, TableNameLock}; +use crate::rpc::ddl::{CommentObjectType, CommentOnTask}; +use crate::rpc::store::PutRequest; + +pub struct CommentOnProcedure { + pub context: DdlContext, + pub data: CommentOnData, +} + +impl CommentOnProcedure { + pub const TYPE_NAME: &'static str = "metasrv-procedure::CommentOn"; + + pub fn new(task: CommentOnTask, context: DdlContext) -> Self { + Self { + context, + data: CommentOnData::new(task), + } + } + + pub fn from_json(json: &str, context: DdlContext) -> ProcedureResult { + let data = serde_json::from_str(json).context(FromJsonSnafu)?; + + Ok(Self { context, data }) + } + + pub async fn on_prepare(&mut self) -> Result { + match self.data.object_type { + CommentObjectType::Table | CommentObjectType::Column => { + self.prepare_table_or_column().await?; + } + CommentObjectType::Flow => { + self.prepare_flow().await?; + } + } + + self.data.state = CommentOnState::UpdateMetadata; + Ok(Status::executing(true)) + } + + async fn prepare_table_or_column(&mut self) -> Result<()> { + let table_name_key = TableNameKey::new( + &self.data.catalog_name, + &self.data.schema_name, + &self.data.object_name, + ); + + let table_id = self + .context + .table_metadata_manager + .table_name_manager() + .get(table_name_key) + .await? + .with_context(|| TableNotFoundSnafu { + table_name: format_full_table_name( + &self.data.catalog_name, + &self.data.schema_name, + &self.data.object_name, + ), + })? + .table_id(); + + let table_info = self + .context + .table_metadata_manager + .table_info_manager() + .get(table_id) + .await? + .context(TableNotFoundSnafu { + table_name: format_full_table_name( + &self.data.catalog_name, + &self.data.schema_name, + &self.data.object_name, + ), + })?; + + // For column comments, validate the column exists + if self.data.object_type == CommentObjectType::Column { + let column_name = self.data.column_name.as_ref().unwrap(); + let column_exists = table_info + .table_info + .meta + .schema + .column_schemas + .iter() + .any(|col| &col.name == column_name); + + ensure!( + column_exists, + UnexpectedSnafu { + err_msg: format!( + "Column `{}` not found in table {}", + column_name, + format_full_table_name( + &self.data.catalog_name, + &self.data.schema_name, + &self.data.object_name, + ), + ), + } + ); + } + + self.data.table_id = Some(table_id); + self.data.table_info = Some(table_info); + + Ok(()) + } + + async fn prepare_flow(&mut self) -> Result<()> { + let flow_name_value = self + .context + .flow_metadata_manager + .flow_name_manager() + .get(&self.data.catalog_name, &self.data.object_name) + .await? + .context(FlowNotFoundSnafu { + flow_name: &self.data.object_name, + })?; + + let flow_id = flow_name_value.flow_id(); + let flow_info = self + .context + .flow_metadata_manager + .flow_info_manager() + .get_raw(flow_id) + .await? + .context(FlowNotFoundSnafu { + flow_name: &self.data.object_name, + })?; + + self.data.flow_id = Some(flow_id); + self.data.flow_info = Some(flow_info); + + Ok(()) + } + + pub async fn on_update_metadata(&mut self) -> Result { + match self.data.object_type { + CommentObjectType::Table => { + self.update_table_comment().await?; + } + CommentObjectType::Column => { + self.update_column_comment().await?; + } + CommentObjectType::Flow => { + self.update_flow_comment().await?; + } + } + + self.data.state = CommentOnState::InvalidateCache; + Ok(Status::executing(true)) + } + + async fn update_table_comment(&mut self) -> Result<()> { + let table_info_value = self.data.table_info.as_ref().unwrap(); + let mut new_table_info = table_info_value.table_info.clone(); + + new_table_info.desc = self.data.comment.clone(); + + // Sync comment to table options + sync_table_comment_option( + &mut new_table_info.meta.options, + new_table_info.desc.as_deref(), + ); + + self.update_table_info(table_info_value, new_table_info) + .await?; + + info!( + "Updated comment for table {}.{}.{}", + self.data.catalog_name, self.data.schema_name, self.data.object_name + ); + + Ok(()) + } + + async fn update_column_comment(&mut self) -> Result<()> { + let table_info_value = self.data.table_info.as_ref().unwrap(); + let mut new_table_info = table_info_value.table_info.clone(); + + let column_name = self.data.column_name.as_ref().unwrap(); + let column_schema = new_table_info + .meta + .schema + .column_schemas + .iter_mut() + .find(|col| &col.name == column_name) + .unwrap(); // Safe: validated in prepare + + update_column_comment_metadata(column_schema, self.data.comment.clone()); + + self.update_table_info(table_info_value, new_table_info) + .await?; + + info!( + "Updated comment for column {}.{}.{}.{}", + self.data.catalog_name, self.data.schema_name, self.data.object_name, column_name + ); + + Ok(()) + } + + async fn update_flow_comment(&mut self) -> Result<()> { + let flow_id = self.data.flow_id.unwrap(); + let flow_info_value = self.data.flow_info.as_ref().unwrap(); + + let mut new_flow_info = flow_info_value.get_inner_ref().clone(); + new_flow_info.comment = self.data.comment.clone().unwrap_or_default(); + new_flow_info.updated_time = Utc::now(); + + let raw_value = serde_json::to_vec(&new_flow_info).context(crate::error::SerdeJsonSnafu)?; + + self.context + .table_metadata_manager + .kv_backend() + .put( + PutRequest::new() + .with_key(FlowInfoKey::new(flow_id).to_bytes()) + .with_value(raw_value), + ) + .await?; + + info!( + "Updated comment for flow {}.{}", + self.data.catalog_name, self.data.object_name + ); + + Ok(()) + } + + async fn update_table_info( + &self, + current_table_info: &DeserializedValueWithBytes, + new_table_info: RawTableInfo, + ) -> Result<()> { + let table_id = current_table_info.table_info.ident.table_id; + let new_table_info_value = current_table_info.update(new_table_info); + let raw_value = new_table_info_value.try_as_raw_value()?; + + self.context + .table_metadata_manager + .kv_backend() + .put( + PutRequest::new() + .with_key(TableInfoKey::new(table_id).to_bytes()) + .with_value(raw_value), + ) + .await?; + + Ok(()) + } + + pub async fn on_invalidate_cache(&mut self) -> Result { + let cache_invalidator = &self.context.cache_invalidator; + + match self.data.object_type { + CommentObjectType::Table | CommentObjectType::Column => { + let table_id = self.data.table_id.unwrap(); + let table_name = TableName::new( + self.data.catalog_name.clone(), + self.data.schema_name.clone(), + self.data.object_name.clone(), + ); + + let cache_ident = vec![ + CacheIdent::TableId(table_id), + CacheIdent::TableName(table_name), + ]; + + cache_invalidator + .invalidate(&Context::default(), &cache_ident) + .await?; + } + CommentObjectType::Flow => { + let flow_id = self.data.flow_id.unwrap(); + let cache_ident = vec![CacheIdent::FlowId(flow_id)]; + + cache_invalidator + .invalidate(&Context::default(), &cache_ident) + .await?; + } + } + + Ok(Status::done()) + } +} + +#[async_trait] +impl Procedure for CommentOnProcedure { + fn type_name(&self) -> &str { + Self::TYPE_NAME + } + + async fn execute(&mut self, _ctx: &ProcedureContext) -> ProcedureResult { + match self.data.state { + CommentOnState::Prepare => self.on_prepare().await, + CommentOnState::UpdateMetadata => self.on_update_metadata().await, + CommentOnState::InvalidateCache => self.on_invalidate_cache().await, + } + .map_err(map_to_procedure_error) + } + + fn dump(&self) -> ProcedureResult { + serde_json::to_string(&self.data).context(ToJsonSnafu) + } + + fn lock_key(&self) -> LockKey { + let catalog = &self.data.catalog_name; + let schema = &self.data.schema_name; + + let lock_key = match self.data.object_type { + CommentObjectType::Table | CommentObjectType::Column => { + vec![ + CatalogLock::Read(catalog).into(), + SchemaLock::read(catalog, schema).into(), + TableNameLock::new(catalog, schema, &self.data.object_name).into(), + ] + } + CommentObjectType::Flow => { + vec![ + CatalogLock::Read(catalog).into(), + FlowNameLock::new(catalog, &self.data.object_name).into(), + ] + } + }; + + LockKey::new(lock_key) + } +} + +#[derive(Debug, Serialize, Deserialize, AsRefStr)] +enum CommentOnState { + Prepare, + UpdateMetadata, + InvalidateCache, +} + +/// The data of comment on procedure. +#[derive(Debug, Serialize, Deserialize)] +pub struct CommentOnData { + state: CommentOnState, + catalog_name: String, + schema_name: String, + object_type: CommentObjectType, + object_name: String, + /// Column name (only for Column comments) + column_name: Option, + comment: Option, + /// Cached table ID (for Table/Column) + #[serde(skip_serializing_if = "Option::is_none")] + table_id: Option, + /// Cached table info (for Table/Column) + #[serde(skip)] + table_info: Option>, + /// Cached flow ID (for Flow) + #[serde(skip_serializing_if = "Option::is_none")] + flow_id: Option, + /// Cached flow info (for Flow) + #[serde(skip)] + flow_info: Option>, +} + +impl CommentOnData { + pub fn new(task: CommentOnTask) -> Self { + Self { + state: CommentOnState::Prepare, + catalog_name: task.catalog_name, + schema_name: task.schema_name, + object_type: task.object_type, + object_name: task.object_name, + column_name: task.column_name, + comment: task.comment, + table_id: None, + table_info: None, + flow_id: None, + flow_info: None, + } + } +} + +fn update_column_comment_metadata( + column_schema: &mut datatypes::schema::ColumnSchema, + comment: Option, +) { + match comment { + Some(value) => { + column_schema + .mut_metadata() + .insert(COLUMN_COMMENT_KEY.to_string(), value); + } + None => { + column_schema.mut_metadata().remove(COLUMN_COMMENT_KEY); + } + } +} + +fn sync_table_comment_option(options: &mut table::requests::TableOptions, comment: Option<&str>) { + match comment { + Some(value) => { + options + .extra_options + .insert(TABLE_COMMENT_KEY.to_string(), value.to_string()); + } + None => { + options.extra_options.remove(TABLE_COMMENT_KEY); + } + } +} diff --git a/src/common/meta/src/ddl_manager.rs b/src/common/meta/src/ddl_manager.rs index 9ade13052d64..fd2150d47ef8 100644 --- a/src/common/meta/src/ddl_manager.rs +++ b/src/common/meta/src/ddl_manager.rs @@ -26,6 +26,7 @@ use store_api::storage::TableId; use crate::ddl::alter_database::AlterDatabaseProcedure; use crate::ddl::alter_logical_tables::AlterLogicalTablesProcedure; use crate::ddl::alter_table::AlterTableProcedure; +use crate::ddl::comment_on::CommentOnProcedure; use crate::ddl::create_database::CreateDatabaseProcedure; use crate::ddl::create_flow::CreateFlowProcedure; use crate::ddl::create_logical_tables::CreateLogicalTablesProcedure; @@ -51,18 +52,18 @@ use crate::rpc::ddl::DdlTask::CreateTrigger; #[cfg(feature = "enterprise")] use crate::rpc::ddl::DdlTask::DropTrigger; use crate::rpc::ddl::DdlTask::{ - AlterDatabase, AlterLogicalTables, AlterTable, CreateDatabase, CreateFlow, CreateLogicalTables, - CreateTable, CreateView, DropDatabase, DropFlow, DropLogicalTables, DropTable, DropView, - TruncateTable, + AlterDatabase, AlterLogicalTables, AlterTable, CommentOn, CreateDatabase, CreateFlow, + CreateLogicalTables, CreateTable, CreateView, DropDatabase, DropFlow, DropLogicalTables, + DropTable, DropView, TruncateTable, }; #[cfg(feature = "enterprise")] use crate::rpc::ddl::trigger::CreateTriggerTask; #[cfg(feature = "enterprise")] use crate::rpc::ddl::trigger::DropTriggerTask; use crate::rpc::ddl::{ - AlterDatabaseTask, AlterTableTask, CreateDatabaseTask, CreateFlowTask, CreateTableTask, - CreateViewTask, DropDatabaseTask, DropFlowTask, DropTableTask, DropViewTask, QueryContext, - SubmitDdlTaskRequest, SubmitDdlTaskResponse, TruncateTableTask, + AlterDatabaseTask, AlterTableTask, CommentOnTask, CreateDatabaseTask, CreateFlowTask, + CreateTableTask, CreateViewTask, DropDatabaseTask, DropFlowTask, DropTableTask, DropViewTask, + QueryContext, SubmitDdlTaskRequest, SubmitDdlTaskResponse, TruncateTableTask, }; use crate::rpc::router::RegionRoute; @@ -181,7 +182,8 @@ impl DdlManager { TruncateTableProcedure, CreateDatabaseProcedure, DropDatabaseProcedure, - DropViewProcedure + DropViewProcedure, + CommentOnProcedure ); for (type_name, loader_factory) in loaders { @@ -397,6 +399,19 @@ impl DdlManager { self.submit_procedure(procedure_with_id).await } + /// Submits and executes a comment on task. + #[tracing::instrument(skip_all)] + pub async fn submit_comment_on_task( + &self, + comment_on_task: CommentOnTask, + ) -> Result<(ProcedureId, Option)> { + let context = self.create_context(); + let procedure = CommentOnProcedure::new(comment_on_task, context); + let procedure_with_id = ProcedureWithId::with_random_id(Box::new(procedure)); + + self.submit_procedure(procedure_with_id).await + } + async fn submit_procedure( &self, procedure_with_id: ProcedureWithId, @@ -465,6 +480,7 @@ impl DdlManager { handle_create_view_task(self, create_view_task).await } DropView(drop_view_task) => handle_drop_view_task(self, drop_view_task).await, + CommentOn(comment_on_task) => handle_comment_on_task(self, comment_on_task).await, #[cfg(feature = "enterprise")] CreateTrigger(create_trigger_task) => { handle_create_trigger_task( @@ -896,6 +912,26 @@ async fn handle_create_view_task( }) } +async fn handle_comment_on_task( + ddl_manager: &DdlManager, + comment_on_task: CommentOnTask, +) -> Result { + let (id, _) = ddl_manager + .submit_comment_on_task(comment_on_task.clone()) + .await?; + + let procedure_id = id.to_string(); + info!( + "Comment on {}.{}.{} is updated via procedure_id {id:?}", + comment_on_task.catalog_name, comment_on_task.schema_name, comment_on_task.object_name + ); + + Ok(SubmitDdlTaskResponse { + key: procedure_id.into(), + ..Default::default() + }) +} + #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/src/common/meta/src/rpc/ddl.rs b/src/common/meta/src/rpc/ddl.rs index d23c943c7708..30c64bf1bb17 100644 --- a/src/common/meta/src/rpc/ddl.rs +++ b/src/common/meta/src/rpc/ddl.rs @@ -23,19 +23,20 @@ use api::v1::alter_database_expr::Kind as PbAlterDatabaseKind; use api::v1::meta::ddl_task_request::Task; use api::v1::meta::{ AlterDatabaseTask as PbAlterDatabaseTask, AlterTableTask as PbAlterTableTask, - AlterTableTasks as PbAlterTableTasks, CreateDatabaseTask as PbCreateDatabaseTask, - CreateFlowTask as PbCreateFlowTask, CreateTableTask as PbCreateTableTask, - CreateTableTasks as PbCreateTableTasks, CreateViewTask as PbCreateViewTask, - DdlTaskRequest as PbDdlTaskRequest, DdlTaskResponse as PbDdlTaskResponse, - DropDatabaseTask as PbDropDatabaseTask, DropFlowTask as PbDropFlowTask, - DropTableTask as PbDropTableTask, DropTableTasks as PbDropTableTasks, - DropViewTask as PbDropViewTask, Partition, ProcedureId, + AlterTableTasks as PbAlterTableTasks, CommentOnTask as PbCommentOnTask, + CreateDatabaseTask as PbCreateDatabaseTask, CreateFlowTask as PbCreateFlowTask, + CreateTableTask as PbCreateTableTask, CreateTableTasks as PbCreateTableTasks, + CreateViewTask as PbCreateViewTask, DdlTaskRequest as PbDdlTaskRequest, + DdlTaskResponse as PbDdlTaskResponse, DropDatabaseTask as PbDropDatabaseTask, + DropFlowTask as PbDropFlowTask, DropTableTask as PbDropTableTask, + DropTableTasks as PbDropTableTasks, DropViewTask as PbDropViewTask, Partition, ProcedureId, TruncateTableTask as PbTruncateTableTask, }; use api::v1::{ - AlterDatabaseExpr, AlterTableExpr, CreateDatabaseExpr, CreateFlowExpr, CreateTableExpr, - CreateViewExpr, DropDatabaseExpr, DropFlowExpr, DropTableExpr, DropViewExpr, EvalInterval, - ExpireAfter, Option as PbOption, QueryContext as PbQueryContext, TruncateTableExpr, + AlterDatabaseExpr, AlterTableExpr, CommentObjectType as PbCommentObjectType, CommentOnExpr, + CreateDatabaseExpr, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropDatabaseExpr, + DropFlowExpr, DropTableExpr, DropViewExpr, EvalInterval, ExpireAfter, Option as PbOption, + QueryContext as PbQueryContext, TruncateTableExpr, }; use base64::Engine as _; use base64::engine::general_purpose; @@ -77,6 +78,7 @@ pub enum DdlTask { DropView(DropViewTask), #[cfg(feature = "enterprise")] CreateTrigger(trigger::CreateTriggerTask), + CommentOn(CommentOnTask), } impl DdlTask { @@ -199,6 +201,11 @@ impl DdlTask { view_info, }) } + + /// Creates a [`DdlTask`] to comment on a table, column, or flow. + pub fn new_comment_on(task: CommentOnTask) -> Self { + DdlTask::CommentOn(task) + } } impl TryFrom for DdlTask { @@ -277,6 +284,7 @@ impl TryFrom for DdlTask { .fail() } } + Task::CommentOnTask(comment_on) => Ok(DdlTask::CommentOn(comment_on.try_into()?)), } } } @@ -331,6 +339,7 @@ impl TryFrom for PbDdlTaskRequest { DdlTask::CreateTrigger(task) => Task::CreateTriggerTask(task.try_into()?), #[cfg(feature = "enterprise")] DdlTask::DropTrigger(task) => Task::DropTriggerTask(task.into()), + DdlTask::CommentOn(task) => Task::CommentOnTask(task.into()), }; Ok(Self { @@ -1260,6 +1269,111 @@ impl From for PbDropFlowTask { } } +/// Represents the ID of the object being commented on (Table or Flow). +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub enum CommentObjectId { + Table(TableId), + Flow(FlowId), +} + +/// Comment on table, column, or flow +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct CommentOnTask { + pub catalog_name: String, + pub schema_name: String, + pub object_type: CommentObjectType, + pub object_name: String, + /// Column name (only for Column comments) + pub column_name: Option, + /// Object ID (Table or Flow) for validation and cache invalidation + pub object_id: Option, + pub comment: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub enum CommentObjectType { + Table, + Column, + Flow, +} + +impl CommentOnTask { + pub fn table_ref(&self) -> TableReference { + TableReference { + catalog: &self.catalog_name, + schema: &self.schema_name, + table: &self.object_name, + } + } +} + +// Proto conversions for CommentObjectType +impl From for PbCommentObjectType { + fn from(object_type: CommentObjectType) -> Self { + match object_type { + CommentObjectType::Table => PbCommentObjectType::Table, + CommentObjectType::Column => PbCommentObjectType::Column, + CommentObjectType::Flow => PbCommentObjectType::Flow, + } + } +} + +impl From for CommentObjectType { + fn from(value: i32) -> Self { + match value { + 0 => CommentObjectType::Table, + 1 => CommentObjectType::Column, + 2 => CommentObjectType::Flow, + _ => CommentObjectType::Table, // Default to Table for unknown values + } + } +} + +// Proto conversions for CommentOnTask +impl TryFrom for CommentOnTask { + type Error = error::Error; + + fn try_from(pb: PbCommentOnTask) -> Result { + let comment_on = pb.comment_on.context(error::InvalidProtoMsgSnafu { + err_msg: "expected comment_on", + })?; + + Ok(CommentOnTask { + catalog_name: comment_on.catalog_name, + schema_name: comment_on.schema_name, + object_type: comment_on.object_type.into(), + object_name: comment_on.object_name, + column_name: if comment_on.column_name.is_empty() { + None + } else { + Some(comment_on.column_name) + }, + comment: if comment_on.comment.is_empty() { + None + } else { + Some(comment_on.comment) + }, + object_id: None, + }) + } +} + +impl From for PbCommentOnTask { + fn from(task: CommentOnTask) -> Self { + let pb_object_type: PbCommentObjectType = task.object_type.into(); + PbCommentOnTask { + comment_on: Some(CommentOnExpr { + catalog_name: task.catalog_name, + schema_name: task.schema_name, + object_type: pb_object_type as i32, + object_name: task.object_name, + column_name: task.column_name.unwrap_or_default(), + comment: task.comment.unwrap_or_default(), + }), + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct QueryContext { pub(crate) current_catalog: String, diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index 58ce538b15e2..bd4c3f577ef7 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -12,351 +12,100 @@ // See the License for the specific language governing permissions and // limitations under the License. -use chrono::Utc; -use common_catalog::format_full_table_name; use common_error::ext::BoxedError; -use common_meta::cache_invalidator::Context; -use common_meta::instruction::CacheIdent; -use common_meta::key::flow::flow_info::{FlowInfoKey, FlowInfoValue}; -use common_meta::key::table_info::{TableInfoKey, TableInfoValue}; -use common_meta::key::{DeserializedValueWithBytes, FlowId, MetadataKey, MetadataValue}; -use common_meta::rpc::store::PutRequest; +use common_meta::procedure_executor::ExecutorContext; +use common_meta::rpc::ddl::{CommentObjectType, CommentOnTask, DdlTask, SubmitDdlTaskRequest}; use common_query::Output; -use datatypes::schema::COMMENT_KEY as COLUMN_COMMENT_KEY; use session::context::QueryContextRef; use session::table_name::table_idents_to_full_name; -use snafu::{GenerateImplicitData, Location, OptionExt, ResultExt}; -use sql::ast::{Ident, ObjectName, ObjectNamePartExt}; +use snafu::ResultExt; +use sql::ast::ObjectNamePartExt; use sql::statements::comment::{Comment, CommentObject}; -use table::metadata::{RawTableInfo, TableId}; -use table::requests::COMMENT_KEY as TABLE_COMMENT_KEY; -use table::table_name::TableName; -use crate::error::{ - CatalogSnafu, ColumnNotFoundSnafu, ExternalSnafu, FlowNotFoundSnafu, InvalidSqlSnafu, - InvalidateTableCacheSnafu, Result, TableMetadataManagerSnafu, TableNotFoundSnafu, -}; +use crate::error::{ExecuteDdlSnafu, ExternalSnafu, InvalidSqlSnafu, Result}; use crate::statement::StatementExecutor; -struct ResolvedTable { - catalog: String, - schema: String, - table: String, - table_id: TableId, - table_name: TableName, - table_info: DeserializedValueWithBytes, -} - impl StatementExecutor { pub async fn comment(&self, stmt: Comment, query_ctx: QueryContextRef) -> Result { - match stmt.object { - CommentObject::Table(table) => { - self.comment_on_table(table, stmt.comment, query_ctx).await - } - CommentObject::Column { table, column } => { - self.comment_on_column(table, column, stmt.comment, query_ctx) - .await - } - CommentObject::Flow(flow) => self.comment_on_flow(flow, stmt.comment, query_ctx).await, - } - } - - async fn comment_on_table( - &self, - table: ObjectName, - comment: Option, - query_ctx: QueryContextRef, - ) -> Result { - let ResolvedTable { - table_id, - table_name, - table_info, - .. - } = self.resolve_table_for_comment(&table, &query_ctx).await?; - - let mut new_table_info = table_info.table_info.clone(); - new_table_info.desc = comment; - sync_table_comment_option( - &mut new_table_info.meta.options, - new_table_info.desc.as_deref(), - ); - - self.update_table_info(&table_info, new_table_info).await?; - - self.invalidate_table_cache_by_name(table_id, table_name) - .await?; - - Ok(Output::new_with_affected_rows(0)) - } - - async fn comment_on_column( - &self, - table: ObjectName, - column: Ident, - comment: Option, - query_ctx: QueryContextRef, - ) -> Result { - let ResolvedTable { - catalog, - schema, - table, - table_id, - table_name, - table_info, - } = self.resolve_table_for_comment(&table, &query_ctx).await?; - - let full_table_name = format_full_table_name(&catalog, &schema, &table); - let mut new_table_info = table_info.table_info.clone(); - - let column_name = column.value.clone(); - let column_schema = new_table_info - .meta - .schema - .column_schemas - .iter_mut() - .find(|col| col.name == column_name) - .with_context(|| ColumnNotFoundSnafu { - msg: format!("{} column `{}`", full_table_name, column_name), - })?; - - update_column_comment_metadata(column_schema, comment); + let comment_on_task = self.create_comment_on_task(stmt, &query_ctx)?; - self.update_table_info(&table_info, new_table_info).await?; - - self.invalidate_table_cache_by_name(table_id, table_name) - .await?; - - Ok(Output::new_with_affected_rows(0)) - } - - async fn comment_on_flow( - &self, - flow_name: ObjectName, - comment: Option, - query_ctx: QueryContextRef, - ) -> Result { - let (catalog_name, flow_name_str) = match &flow_name.0[..] { - [flow] => ( - query_ctx.current_catalog().to_string(), - flow.to_string_unquoted(), - ), - [catalog, flow] => (catalog.to_string_unquoted(), flow.to_string_unquoted()), - _ => { - return InvalidSqlSnafu { - err_msg: format!( - "expect flow name to be . or , actual: {flow_name}" - ), - } - .fail(); - } + let request = SubmitDdlTaskRequest { + task: DdlTask::new_comment_on(comment_on_task), + query_context: query_ctx, }; - let flow_name_val = self - .flow_metadata_manager - .flow_name_manager() - .get(&catalog_name, &flow_name_str) - .await - .context(TableMetadataManagerSnafu)? - .context(FlowNotFoundSnafu { - flow_name: &flow_name_str, - })?; - - let flow_id = flow_name_val.flow_id(); - let flow_info = self - .flow_metadata_manager - .flow_info_manager() - .get_raw(flow_id) + self.procedure_executor + .submit_ddl_task(&ExecutorContext::default(), request) .await - .context(TableMetadataManagerSnafu)? - .context(FlowNotFoundSnafu { - flow_name: &flow_name_str, - })?; - - let mut new_flow_info = flow_info.get_inner_ref().clone(); - new_flow_info.comment = comment.unwrap_or_default(); - new_flow_info.updated_time = Utc::now(); - - self.update_flow_metadata(flow_id, new_flow_info).await?; - - Ok(Output::new_with_affected_rows(0)) + .context(ExecuteDdlSnafu) + .map(|_| Output::new_with_affected_rows(0)) } - async fn resolve_table_for_comment( + fn create_comment_on_task( &self, - table: &ObjectName, + stmt: Comment, query_ctx: &QueryContextRef, - ) -> Result { - let (catalog_name, schema_name, table_name) = table_idents_to_full_name(table, query_ctx) - .map_err(BoxedError::new) - .context(ExternalSnafu)?; - - let full_table_name = format_full_table_name(&catalog_name, &schema_name, &table_name); - - let table_ref = self - .catalog_manager - .table(&catalog_name, &schema_name, &table_name, Some(query_ctx)) - .await - .context(CatalogSnafu)? - .with_context(|| TableNotFoundSnafu { - table_name: full_table_name.clone(), - })?; - - let table_id = table_ref.table_info().table_id(); - let table_info = self - .table_metadata_manager - .table_info_manager() - .get(table_id) - .await - .context(TableMetadataManagerSnafu)? - .context(TableNotFoundSnafu { - table_name: full_table_name, - })?; - - Ok(ResolvedTable { - catalog: catalog_name.clone(), - schema: schema_name.clone(), - table: table_name.clone(), - table_id, - table_name: TableName::new(catalog_name, schema_name, table_name), - table_info, - }) - } - - async fn invalidate_table_cache_by_name( - &self, - table_id: TableId, - table_name: TableName, - ) -> Result<()> { - let cache_ident = [ - CacheIdent::TableId(table_id), - CacheIdent::TableName(table_name), - ]; - self.cache_invalidator - .invalidate(&Context::default(), &cache_ident) - .await - .context(InvalidateTableCacheSnafu)?; - Ok(()) - } - - async fn update_table_info( - &self, - current_table_info: &DeserializedValueWithBytes, - new_table_info: RawTableInfo, - ) -> Result<()> { - let table_id = current_table_info.table_info.ident.table_id; - let new_table_info_value = current_table_info.update(new_table_info); - let raw_value = new_table_info_value - .try_as_raw_value() - .context(TableMetadataManagerSnafu)?; - - self.table_metadata_manager - .kv_backend() - .put( - PutRequest::new() - .with_key(TableInfoKey::new(table_id).to_bytes()) - .with_value(raw_value), - ) - .await - .context(TableMetadataManagerSnafu)?; - - Ok(()) - } - - async fn update_flow_metadata( - &self, - flow_id: FlowId, - new_flow_info: FlowInfoValue, - ) -> Result<()> { - let raw_value = serde_json::to_vec(&new_flow_info) - .map_err(|err| common_meta::error::Error::SerdeJson { - error: err, - location: Location::generate(), - }) - .context(TableMetadataManagerSnafu)?; - - self.table_metadata_manager - .kv_backend() - .put( - PutRequest::new() - .with_key(FlowInfoKey::new(flow_id).to_bytes()) - .with_value(raw_value), - ) - .await - .context(TableMetadataManagerSnafu)?; - - Ok(()) - } -} - -fn update_column_comment_metadata( - column_schema: &mut datatypes::schema::ColumnSchema, - comment: Option, -) { - match comment { - Some(value) => { - column_schema - .mut_metadata() - .insert(COLUMN_COMMENT_KEY.to_string(), value); - } - None => { - column_schema.mut_metadata().remove(COLUMN_COMMENT_KEY); - } - } -} - -fn sync_table_comment_option(options: &mut table::requests::TableOptions, comment: Option<&str>) { - match comment { - Some(value) => { - options - .extra_options - .insert(TABLE_COMMENT_KEY.to_string(), value.to_string()); - } - None => { - options.extra_options.remove(TABLE_COMMENT_KEY); + ) -> Result { + match stmt.object { + CommentObject::Table(table) => { + let (catalog_name, schema_name, table_name) = + table_idents_to_full_name(&table, query_ctx) + .map_err(BoxedError::new) + .context(ExternalSnafu)?; + + Ok(CommentOnTask { + catalog_name, + schema_name, + object_type: CommentObjectType::Table, + object_name: table_name, + column_name: None, + object_id: None, + comment: stmt.comment, + }) + } + CommentObject::Column { table, column } => { + let (catalog_name, schema_name, table_name) = + table_idents_to_full_name(&table, query_ctx) + .map_err(BoxedError::new) + .context(ExternalSnafu)?; + + Ok(CommentOnTask { + catalog_name, + schema_name, + object_type: CommentObjectType::Column, + object_name: table_name, + column_name: Some(column.value), + object_id: None, + comment: stmt.comment, + }) + } + CommentObject::Flow(flow_name) => { + let (catalog_name, flow_name_str) = match &flow_name.0[..] { + [flow] => ( + query_ctx.current_catalog().to_string(), + flow.to_string_unquoted(), + ), + [catalog, flow] => (catalog.to_string_unquoted(), flow.to_string_unquoted()), + _ => { + return InvalidSqlSnafu { + err_msg: format!( + "expect flow name to be . or , actual: {flow_name}" + ), + } + .fail(); + } + }; + + Ok(CommentOnTask { + catalog_name, + schema_name: String::new(), // Flow doesn't use schema + object_type: CommentObjectType::Flow, + object_name: flow_name_str, + column_name: None, + object_id: None, + comment: stmt.comment, + }) + } } } } - -#[cfg(test)] -mod tests { - use datatypes::data_type::ConcreteDataType; - use datatypes::schema::{COMMENT_KEY as COLUMN_COMMENT_KEY, ColumnSchema}; - use table::requests::TableOptions; - - use super::{sync_table_comment_option, update_column_comment_metadata}; - - #[test] - fn test_update_column_comment_metadata_uses_schema_key() { - let mut column_schema = ColumnSchema::new("col", ConcreteDataType::string_datatype(), true); - - update_column_comment_metadata(&mut column_schema, Some("note".to_string())); - - assert_eq!( - column_schema - .metadata() - .get(COLUMN_COMMENT_KEY) - .map(|value| value.as_str()), - Some("note") - ); - // Make sure it's not the `COMMENT_KEY` from `table::request` - assert!(column_schema.metadata().get("comment").is_none()); - - update_column_comment_metadata(&mut column_schema, None); - - assert!(column_schema.metadata().get(COLUMN_COMMENT_KEY).is_none()); - } - - #[test] - fn test_sync_table_comment_option() { - let mut options = TableOptions::default(); - - sync_table_comment_option(&mut options, Some("table comment")); - assert_eq!( - options.extra_options.get(super::TABLE_COMMENT_KEY), - Some(&"table comment".to_string()) - ); - - sync_table_comment_option(&mut options, None); - assert!(!options.extra_options.contains_key(super::TABLE_COMMENT_KEY)); - } -} From a38274fc529c90c903ff4d4896e82e5a9f0fe3a5 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Sat, 11 Oct 2025 21:06:23 +0800 Subject: [PATCH 13/21] update proto Signed-off-by: Ruihang Xia --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/api/src/helper.rs | 1 + src/common/meta/src/rpc/ddl.rs | 2 +- src/frontend/src/instance/grpc.rs | 3 +++ 5 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6e88493e928e..bb17ea942ccc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5325,7 +5325,7 @@ dependencies = [ [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=d75496d5d09dedcd0edcade57ccf0a522f4393ae#d75496d5d09dedcd0edcade57ccf0a522f4393ae" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=295be556573f7f90b6b004d046d3f2f127a5739f#295be556573f7f90b6b004d046d3f2f127a5739f" dependencies = [ "prost 0.13.5", "prost-types 0.13.5", diff --git a/Cargo.toml b/Cargo.toml index 1185c9b2d573..da7bdc4943d5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -147,7 +147,7 @@ etcd-client = { git = "https://github.com/GreptimeTeam/etcd-client", rev = "f62d fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "d75496d5d09dedcd0edcade57ccf0a522f4393ae" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "295be556573f7f90b6b004d046d3f2f127a5739f" } hex = "0.4" http = "1" humantime = "2.1" diff --git a/src/api/src/helper.rs b/src/api/src/helper.rs index a091d997997b..db25acfd0ec6 100644 --- a/src/api/src/helper.rs +++ b/src/api/src/helper.rs @@ -603,6 +603,7 @@ fn ddl_request_type(request: &DdlRequest) -> &'static str { Some(Expr::CreateView(_)) => "ddl.create_view", Some(Expr::DropView(_)) => "ddl.drop_view", Some(Expr::AlterDatabase(_)) => "ddl.alter_database", + Some(Expr::CommentOn(_)) => "ddl.comment_on", None => "ddl.empty", } } diff --git a/src/common/meta/src/rpc/ddl.rs b/src/common/meta/src/rpc/ddl.rs index 02c7cd30b51d..a1af418a94f2 100644 --- a/src/common/meta/src/rpc/ddl.rs +++ b/src/common/meta/src/rpc/ddl.rs @@ -1298,7 +1298,7 @@ pub enum CommentObjectType { } impl CommentOnTask { - pub fn table_ref(&self) -> TableReference { + pub fn table_ref(&self) -> TableReference<'_> { TableReference { catalog: &self.catalog_name, schema: &self.schema_name, diff --git a/src/frontend/src/instance/grpc.rs b/src/frontend/src/instance/grpc.rs index 9eeb57ce0131..efc24bd5968f 100644 --- a/src/frontend/src/instance/grpc.rs +++ b/src/frontend/src/instance/grpc.rs @@ -330,6 +330,9 @@ fn fill_catalog_and_schema_from_context(ddl_expr: &mut DdlExpr, ctx: &QueryConte Expr::DropView(expr) => { check_and_fill!(expr); } + Expr::CommentOn(expr) => { + check_and_fill!(expr); + } } } From e1d1c4315811729d1ac65affbea1f993d747d52e Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 13 Oct 2025 10:56:27 +0800 Subject: [PATCH 14/21] grpc support Signed-off-by: Ruihang Xia --- src/frontend/src/instance/grpc.rs | 5 +++ src/operator/src/statement/comment.rs | 59 ++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/frontend/src/instance/grpc.rs b/src/frontend/src/instance/grpc.rs index efc24bd5968f..129767f177d7 100644 --- a/src/frontend/src/instance/grpc.rs +++ b/src/frontend/src/instance/grpc.rs @@ -230,6 +230,11 @@ impl GrpcQueryHandler for Instance { DdlExpr::DropView(_) => { todo!("implemented in the following PR") } + DdlExpr::CommentOn(expr) => { + self.statement_executor + .comment_by_expr(expr, ctx.clone()) + .await? + } } } }; diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index bd4c3f577ef7..e42334bacf9c 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use api::v1::CommentOnExpr; use common_error::ext::BoxedError; use common_meta::procedure_executor::ExecutorContext; use common_meta::rpc::ddl::{CommentObjectType, CommentOnTask, DdlTask, SubmitDdlTaskRequest}; @@ -27,7 +28,7 @@ use crate::statement::StatementExecutor; impl StatementExecutor { pub async fn comment(&self, stmt: Comment, query_ctx: QueryContextRef) -> Result { - let comment_on_task = self.create_comment_on_task(stmt, &query_ctx)?; + let comment_on_task = self.create_comment_on_task_from_stmt(stmt, &query_ctx)?; let request = SubmitDdlTaskRequest { task: DdlTask::new_comment_on(comment_on_task), @@ -41,7 +42,61 @@ impl StatementExecutor { .map(|_| Output::new_with_affected_rows(0)) } - fn create_comment_on_task( + pub async fn comment_by_expr( + &self, + expr: CommentOnExpr, + query_ctx: QueryContextRef, + ) -> Result { + let comment_on_task = self.create_comment_on_task_from_expr(expr)?; + + let request = SubmitDdlTaskRequest { + task: DdlTask::new_comment_on(comment_on_task), + query_context: query_ctx, + }; + + self.procedure_executor + .submit_ddl_task(&ExecutorContext::default(), request) + .await + .context(ExecuteDdlSnafu) + .map(|_| Output::new_with_affected_rows(0)) + } + + fn create_comment_on_task_from_expr(&self, expr: CommentOnExpr) -> Result { + let object_type = match expr.object_type { + 0 => CommentObjectType::Table, + 1 => CommentObjectType::Column, + 2 => CommentObjectType::Flow, + _ => { + return InvalidSqlSnafu { + err_msg: format!( + "Invalid CommentObjectType value: {}. Valid values are: 0 (Table), 1 (Column), 2 (Flow)", + expr.object_type + ), + } + .fail(); + } + }; + + Ok(CommentOnTask { + catalog_name: expr.catalog_name, + schema_name: expr.schema_name, + object_type, + object_name: expr.object_name, + column_name: if expr.column_name.is_empty() { + None + } else { + Some(expr.column_name) + }, + object_id: None, + comment: if expr.comment.is_empty() { + None + } else { + Some(expr.comment) + }, + }) + } + + fn create_comment_on_task_from_stmt( &self, stmt: Comment, query_ctx: &QueryContextRef, From e9a62b45c7bf0a22645744f7677766d6dea0d0a9 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 13 Oct 2025 11:08:44 +0800 Subject: [PATCH 15/21] Apply suggestions from code review Co-authored-by: dennis zhuang Co-authored-by: LFC <990479+MichaelScofield@users.noreply.github.com> Signed-off-by: Ruihang Xia --- src/operator/src/statement/comment.rs | 10 ++++++++++ src/sql/src/parsers/comment_parser.rs | 6 +++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/operator/src/statement/comment.rs b/src/operator/src/statement/comment.rs index e42334bacf9c..d82d059ad9b8 100644 --- a/src/operator/src/statement/comment.rs +++ b/src/operator/src/statement/comment.rs @@ -27,6 +27,16 @@ use crate::error::{ExecuteDdlSnafu, ExternalSnafu, InvalidSqlSnafu, Result}; use crate::statement::StatementExecutor; impl StatementExecutor { + /// Adds a comment to a database object (table, column, or flow). + /// + /// # Arguments + /// + /// * `stmt`: A `Comment` struct containing the object to comment on and the comment text. + /// * `query_ctx`: A `QueryContextRef` providing contextual information for the query. + /// + /// # Returns + /// + /// A `Result` containing the `Output` of the operation, or an error if the operation fails. pub async fn comment(&self, stmt: Comment, query_ctx: QueryContextRef) -> Result { let comment_on_task = self.create_comment_on_task_from_stmt(stmt, &query_ctx)?; diff --git a/src/sql/src/parsers/comment_parser.rs b/src/sql/src/parsers/comment_parser.rs index edf3def9213d..ea3df03bc387 100644 --- a/src/sql/src/parsers/comment_parser.rs +++ b/src/sql/src/parsers/comment_parser.rs @@ -34,7 +34,7 @@ impl ParserContext<'_> { let target_token = self.parser.next_token(); let comment = match target_token.token { Token::Word(word) if word.keyword == Keyword::TABLE => { - let raw_table = self.parse_object_name().context(error::UnexpectedSnafu { + let raw_table = self.parse_object_name().with_context(|_| error::UnexpectedSnafu { expected: "a table name", actual: self.peek_token_as_string(), })?; @@ -47,7 +47,7 @@ impl ParserContext<'_> { Token::Word(word) if word.keyword == Keyword::NoKeyword && word.value.eq_ignore_ascii_case(FLOW) => { - let raw_flow = self.parse_object_name().context(error::UnexpectedSnafu { + let raw_flow = self.parse_object_name().with_context(|_| error::UnexpectedSnafu { expected: "a flow name", actual: self.peek_token_as_string(), })?; @@ -78,7 +78,7 @@ impl ParserContext<'_> { } fn parse_column_comment_target(&mut self) -> Result { - let raw = self.parse_object_name().context(error::UnexpectedSnafu { + let raw = self.parse_object_name().with_context(|_| error::UnexpectedSnafu { expected: "a column reference", actual: self.peek_token_as_string(), })?; From 785031e83328bb94bd2e996d91d05d2945099899 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 13 Oct 2025 11:18:18 +0800 Subject: [PATCH 16/21] try from pb struct Signed-off-by: Ruihang Xia --- src/common/meta/src/rpc/ddl.rs | 22 +++++++++++++------- src/sql/src/parsers/comment_parser.rs | 30 ++++++++++++++++----------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/src/common/meta/src/rpc/ddl.rs b/src/common/meta/src/rpc/ddl.rs index a1af418a94f2..12f055d083fd 100644 --- a/src/common/meta/src/rpc/ddl.rs +++ b/src/common/meta/src/rpc/ddl.rs @@ -1318,13 +1318,21 @@ impl From for PbCommentObjectType { } } -impl From for CommentObjectType { - fn from(value: i32) -> Self { +impl TryFrom for CommentObjectType { + type Error = error::Error; + + fn try_from(value: i32) -> Result { match value { - 0 => CommentObjectType::Table, - 1 => CommentObjectType::Column, - 2 => CommentObjectType::Flow, - _ => CommentObjectType::Table, // Default to Table for unknown values + 0 => Ok(CommentObjectType::Table), + 1 => Ok(CommentObjectType::Column), + 2 => Ok(CommentObjectType::Flow), + _ => error::InvalidProtoMsgSnafu { + err_msg: format!( + "Invalid CommentObjectType value: {}. Valid values are: 0 (Table), 1 (Column), 2 (Flow)", + value + ), + } + .fail(), } } } @@ -1341,7 +1349,7 @@ impl TryFrom for CommentOnTask { Ok(CommentOnTask { catalog_name: comment_on.catalog_name, schema_name: comment_on.schema_name, - object_type: comment_on.object_type.into(), + object_type: comment_on.object_type.try_into()?, object_name: comment_on.object_name, column_name: if comment_on.column_name.is_empty() { None diff --git a/src/sql/src/parsers/comment_parser.rs b/src/sql/src/parsers/comment_parser.rs index ea3df03bc387..489d191d57f9 100644 --- a/src/sql/src/parsers/comment_parser.rs +++ b/src/sql/src/parsers/comment_parser.rs @@ -34,10 +34,12 @@ impl ParserContext<'_> { let target_token = self.parser.next_token(); let comment = match target_token.token { Token::Word(word) if word.keyword == Keyword::TABLE => { - let raw_table = self.parse_object_name().with_context(|_| error::UnexpectedSnafu { - expected: "a table name", - actual: self.peek_token_as_string(), - })?; + let raw_table = + self.parse_object_name() + .with_context(|_| error::UnexpectedSnafu { + expected: "a table name", + actual: self.peek_token_as_string(), + })?; let table = Self::canonicalize_object_name(raw_table); CommentObject::Table(table) } @@ -47,10 +49,12 @@ impl ParserContext<'_> { Token::Word(word) if word.keyword == Keyword::NoKeyword && word.value.eq_ignore_ascii_case(FLOW) => { - let raw_flow = self.parse_object_name().with_context(|_| error::UnexpectedSnafu { - expected: "a flow name", - actual: self.peek_token_as_string(), - })?; + let raw_flow = + self.parse_object_name() + .with_context(|_| error::UnexpectedSnafu { + expected: "a flow name", + actual: self.peek_token_as_string(), + })?; let flow = Self::canonicalize_object_name(raw_flow); CommentObject::Flow(flow) } @@ -78,10 +82,12 @@ impl ParserContext<'_> { } fn parse_column_comment_target(&mut self) -> Result { - let raw = self.parse_object_name().with_context(|_| error::UnexpectedSnafu { - expected: "a column reference", - actual: self.peek_token_as_string(), - })?; + let raw = self + .parse_object_name() + .with_context(|_| error::UnexpectedSnafu { + expected: "a column reference", + actual: self.peek_token_as_string(), + })?; let canonical = Self::canonicalize_object_name(raw); let mut parts = canonical.0; From 6b7648615dfe7d5767c4754d616d2d7ce44777d4 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 13 Oct 2025 11:20:04 +0800 Subject: [PATCH 17/21] doc comment Signed-off-by: Ruihang Xia --- src/sql/src/statements/comment.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/sql/src/statements/comment.rs b/src/sql/src/statements/comment.rs index ab8bae56a26a..ec0f8d37b679 100644 --- a/src/sql/src/statements/comment.rs +++ b/src/sql/src/statements/comment.rs @@ -19,6 +19,15 @@ use sqlparser_derive::{Visit, VisitMut}; use crate::ast::{Ident, ObjectName}; +/// Represents a SQL COMMENT statement for adding or removing comments on database objects. +/// +/// # Examples +/// +/// ```sql +/// COMMENT ON TABLE my_table IS 'This is a table comment'; +/// COMMENT ON COLUMN my_table.my_column IS 'This is a column comment'; +/// COMMENT ON FLOW my_flow IS NULL; +/// ``` #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] pub struct Comment { pub object: CommentObject, From a378ebf66ed03ff7a457d2af2bbbfe316301d5b0 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 13 Oct 2025 16:10:11 +0800 Subject: [PATCH 18/21] check unchanged fast case Signed-off-by: Ruihang Xia --- src/common/meta/src/ddl/comment_on.rs | 70 +++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/src/common/meta/src/ddl/comment_on.rs b/src/common/meta/src/ddl/comment_on.rs index f5edd0504df5..92c0e7501bfa 100644 --- a/src/common/meta/src/ddl/comment_on.rs +++ b/src/common/meta/src/ddl/comment_on.rs @@ -71,6 +71,34 @@ impl CommentOnProcedure { } } + // Fast path: if comment is unchanged, skip update + if self.data.is_unchanged { + let object_desc = match self.data.object_type { + CommentObjectType::Table => format!( + "table {}", + format_full_table_name( + &self.data.catalog_name, + &self.data.schema_name, + &self.data.object_name, + ) + ), + CommentObjectType::Column => format!( + "column {}.{}", + format_full_table_name( + &self.data.catalog_name, + &self.data.schema_name, + &self.data.object_name, + ), + self.data.column_name.as_ref().unwrap() + ), + CommentObjectType::Flow => { + format!("flow {}.{}", self.data.catalog_name, self.data.object_name) + } + }; + info!("Comment unchanged for {}, skipping update", object_desc); + return Ok(Status::done()); + } + self.data.state = CommentOnState::UpdateMetadata; Ok(Status::executing(true)) } @@ -139,6 +167,36 @@ impl CommentOnProcedure { } self.data.table_id = Some(table_id); + + // Check if comment is unchanged for early exit optimization + match self.data.object_type { + CommentObjectType::Table => { + let current_comment = &table_info.table_info.desc; + if &self.data.comment == current_comment { + self.data.is_unchanged = true; + } + } + CommentObjectType::Column => { + let column_name = self.data.column_name.as_ref().unwrap(); + let column_schema = table_info + .table_info + .meta + .schema + .column_schemas + .iter() + .find(|col| &col.name == column_name) + .unwrap(); // Safe: validated above + + let current_comment = column_schema.metadata().get(COLUMN_COMMENT_KEY); + if self.data.comment.as_deref() == current_comment.map(String::as_str) { + self.data.is_unchanged = true; + } + } + CommentObjectType::Flow => { + // this branch is handled in `prepare_flow` + } + } + self.data.table_info = Some(table_info); Ok(()) @@ -167,6 +225,14 @@ impl CommentOnProcedure { })?; self.data.flow_id = Some(flow_id); + + // Check if comment is unchanged for early exit optimization + let current_comment = &flow_info.get_inner_ref().comment; + let new_comment = self.data.comment.as_deref().unwrap_or(""); + if new_comment == current_comment.as_str() { + self.data.is_unchanged = true; + } + self.data.flow_info = Some(flow_info); Ok(()) @@ -396,6 +462,9 @@ pub struct CommentOnData { /// Cached flow info (for Flow) #[serde(skip)] flow_info: Option>, + /// Whether the comment is unchanged (optimization for early exit) + #[serde(skip)] + is_unchanged: bool, } impl CommentOnData { @@ -412,6 +481,7 @@ impl CommentOnData { table_info: None, flow_id: None, flow_info: None, + is_unchanged: false, } } } From 4b8d9f2c824618fe0a603c1bc513e022b071cea0 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 13 Oct 2025 16:27:26 +0800 Subject: [PATCH 19/21] tune errors Signed-off-by: Ruihang Xia --- src/common/meta/src/ddl/comment_on.rs | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/common/meta/src/ddl/comment_on.rs b/src/common/meta/src/ddl/comment_on.rs index 92c0e7501bfa..9dd46d6da9fe 100644 --- a/src/common/meta/src/ddl/comment_on.rs +++ b/src/common/meta/src/ddl/comment_on.rs @@ -30,7 +30,7 @@ use table::table_name::TableName; use crate::cache_invalidator::Context; use crate::ddl::DdlContext; use crate::ddl::utils::map_to_procedure_error; -use crate::error::{FlowNotFoundSnafu, Result, TableNotFoundSnafu, UnexpectedSnafu}; +use crate::error::{ColumnNotFoundSnafu, FlowNotFoundSnafu, Result, TableNotFoundSnafu}; use crate::instruction::CacheIdent; use crate::key::flow::flow_info::{FlowInfoKey, FlowInfoValue}; use crate::key::table_info::{TableInfoKey, TableInfoValue}; @@ -131,7 +131,7 @@ impl CommentOnProcedure { .table_info_manager() .get(table_id) .await? - .context(TableNotFoundSnafu { + .with_context(|| TableNotFoundSnafu { table_name: format_full_table_name( &self.data.catalog_name, &self.data.schema_name, @@ -152,16 +152,9 @@ impl CommentOnProcedure { ensure!( column_exists, - UnexpectedSnafu { - err_msg: format!( - "Column `{}` not found in table {}", - column_name, - format_full_table_name( - &self.data.catalog_name, - &self.data.schema_name, - &self.data.object_name, - ), - ), + ColumnNotFoundSnafu { + column_name, + column_id: 0u32, // column_id is not known here } ); } @@ -209,7 +202,7 @@ impl CommentOnProcedure { .flow_name_manager() .get(&self.data.catalog_name, &self.data.object_name) .await? - .context(FlowNotFoundSnafu { + .with_context(|| FlowNotFoundSnafu { flow_name: &self.data.object_name, })?; @@ -220,7 +213,7 @@ impl CommentOnProcedure { .flow_info_manager() .get_raw(flow_id) .await? - .context(FlowNotFoundSnafu { + .with_context(|| FlowNotFoundSnafu { flow_name: &self.data.object_name, })?; From cbe52ac7f18f38a8a8cebe35de4f68f7ee5e0f5e Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Fri, 5 Dec 2025 13:04:23 +0800 Subject: [PATCH 20/21] fix merge error Signed-off-by: Ruihang Xia --- src/sql/src/parsers/comment_parser.rs | 15 +++++++++------ tests-integration/tests/http.rs | 4 ++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/sql/src/parsers/comment_parser.rs b/src/sql/src/parsers/comment_parser.rs index 489d191d57f9..bd2cb53c5d96 100644 --- a/src/sql/src/parsers/comment_parser.rs +++ b/src/sql/src/parsers/comment_parser.rs @@ -40,7 +40,7 @@ impl ParserContext<'_> { expected: "a table name", actual: self.peek_token_as_string(), })?; - let table = Self::canonicalize_object_name(raw_table); + let table = Self::canonicalize_object_name(raw_table)?; CommentObject::Table(table) } Token::Word(word) if word.keyword == Keyword::COLUMN => { @@ -55,7 +55,7 @@ impl ParserContext<'_> { expected: "a flow name", actual: self.peek_token_as_string(), })?; - let flow = Self::canonicalize_object_name(raw_flow); + let flow = Self::canonicalize_object_name(raw_flow)?; CommentObject::Flow(flow) } _ => return self.expected("TABLE, COLUMN or FLOW", target_token), @@ -88,7 +88,7 @@ impl ParserContext<'_> { expected: "a column reference", actual: self.peek_token_as_string(), })?; - let canonical = Self::canonicalize_object_name(raw); + let canonical = Self::canonicalize_object_name(raw)?; let mut parts = canonical.0; ensure!( @@ -99,15 +99,18 @@ impl ParserContext<'_> { ); let column_part = parts.pop().unwrap(); - let ObjectNamePart::Identifier(column_ident) = column_part; + let ObjectNamePart::Identifier(column_ident) = column_part else { + unreachable!("canonicalized object name should only contain identifiers"); + }; let column = ParserContext::canonicalize_identifier(column_ident); let mut table_idents: Vec = Vec::with_capacity(parts.len()); for part in parts { match part { - ObjectNamePart::Identifier(ident) => { - table_idents.push(ident); + ObjectNamePart::Identifier(ident) => table_idents.push(ident), + ObjectNamePart::Function(_) => { + unreachable!("canonicalized object name should only contain identifiers") } } } diff --git a/tests-integration/tests/http.rs b/tests-integration/tests/http.rs index f70a5cd83c75..9940040b59ac 100644 --- a/tests-integration/tests/http.rs +++ b/tests-integration/tests/http.rs @@ -6214,7 +6214,7 @@ pub async fn test_jaeger_query_api_for_trace_v1(store_type: StorageType) { .await; assert_eq!(StatusCode::OK, res.status()); - let trace_table_sql = "[[\"mytable\",\"CREATE TABLE IF NOT EXISTS \\\"mytable\\\" (\\n \\\"timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"timestamp_end\\\" TIMESTAMP(9) NULL,\\n \\\"duration_nano\\\" BIGINT UNSIGNED NULL,\\n \\\"parent_span_id\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"trace_id\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"span_id\\\" STRING NULL,\\n \\\"span_kind\\\" STRING NULL,\\n \\\"span_name\\\" STRING NULL,\\n \\\"span_status_code\\\" STRING NULL,\\n \\\"span_status_message\\\" STRING NULL,\\n \\\"trace_state\\\" STRING NULL,\\n \\\"scope_name\\\" STRING NULL,\\n \\\"scope_version\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"span_attributes.operation.type\\\" STRING NULL,\\n \\\"span_attributes.net.peer.ip\\\" STRING NULL,\\n \\\"span_attributes.peer.service\\\" STRING NULL,\\n \\\"span_events\\\" JSON NULL,\\n \\\"span_links\\\" JSON NULL,\\n TIME INDEX (\\\"timestamp\\\"),\\n PRIMARY KEY (\\\"service_name\\\")\\n)\\nPARTITION ON COLUMNS (\\\"trace_id\\\") (\\n trace_id < '1',\\n trace_id >= '1' AND trace_id < '2',\\n trace_id >= '2' AND trace_id < '3',\\n trace_id >= '3' AND trace_id < '4',\\n trace_id >= '4' AND trace_id < '5',\\n trace_id >= '5' AND trace_id < '6',\\n trace_id >= '6' AND trace_id < '7',\\n trace_id >= '7' AND trace_id < '8',\\n trace_id >= '8' AND trace_id < '9',\\n trace_id >= '9' AND trace_id < 'a',\\n trace_id >= 'a' AND trace_id < 'b',\\n trace_id >= 'b' AND trace_id < 'c',\\n trace_id >= 'c' AND trace_id < 'd',\\n trace_id >= 'd' AND trace_id < 'e',\\n trace_id >= 'e' AND trace_id < 'f',\\n trace_id >= 'f'\\n)\\nENGINE=mito\\nWITH(\\n append_mode = 'true',\\n table_data_model = 'greptime_trace_v1',\\n ttl = '7days'\\n)\"]]"; + let trace_table_sql = "[[\"mytable\",\"CREATE TABLE IF NOT EXISTS \\\"mytable\\\" (\\n \\\"timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"timestamp_end\\\" TIMESTAMP(9) NULL,\\n \\\"duration_nano\\\" BIGINT UNSIGNED NULL,\\n \\\"parent_span_id\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"trace_id\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"span_id\\\" STRING NULL,\\n \\\"span_kind\\\" STRING NULL,\\n \\\"span_name\\\" STRING NULL,\\n \\\"span_status_code\\\" STRING NULL,\\n \\\"span_status_message\\\" STRING NULL,\\n \\\"trace_state\\\" STRING NULL,\\n \\\"scope_name\\\" STRING NULL,\\n \\\"scope_version\\\" STRING NULL,\\n \\\"service_name\\\" STRING NULL SKIPPING INDEX WITH(false_positive_rate = '0.01', granularity = '10240', type = 'BLOOM'),\\n \\\"span_attributes.operation.type\\\" STRING NULL,\\n \\\"span_attributes.net.peer.ip\\\" STRING NULL,\\n \\\"span_attributes.peer.service\\\" STRING NULL,\\n \\\"span_events\\\" JSON NULL,\\n \\\"span_links\\\" JSON NULL,\\n TIME INDEX (\\\"timestamp\\\"),\\n PRIMARY KEY (\\\"service_name\\\")\\n)\\nPARTITION ON COLUMNS (\\\"trace_id\\\") (\\n trace_id < '1',\\n trace_id >= '1' AND trace_id < '2',\\n trace_id >= '2' AND trace_id < '3',\\n trace_id >= '3' AND trace_id < '4',\\n trace_id >= '4' AND trace_id < '5',\\n trace_id >= '5' AND trace_id < '6',\\n trace_id >= '6' AND trace_id < '7',\\n trace_id >= '7' AND trace_id < '8',\\n trace_id >= '8' AND trace_id < '9',\\n trace_id >= '9' AND trace_id < 'a',\\n trace_id >= 'a' AND trace_id < 'b',\\n trace_id >= 'b' AND trace_id < 'c',\\n trace_id >= 'c' AND trace_id < 'd',\\n trace_id >= 'd' AND trace_id < 'e',\\n trace_id >= 'e' AND trace_id < 'f',\\n trace_id >= 'f'\\n)\\nENGINE=mito\\nWITH(\\n 'comment' = 'Created on insertion',\\n append_mode = 'true',\\n table_data_model = 'greptime_trace_v1',\\n ttl = '7days'\\n)\"]]"; validate_data( "trace_v1_create_table", &client, @@ -6223,7 +6223,7 @@ pub async fn test_jaeger_query_api_for_trace_v1(store_type: StorageType) { ) .await; - let trace_meta_table_sql = "[[\"mytable_services\",\"CREATE TABLE IF NOT EXISTS \\\"mytable_services\\\" (\\n \\\"timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"service_name\\\" STRING NULL,\\n TIME INDEX (\\\"timestamp\\\"),\\n PRIMARY KEY (\\\"service_name\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n append_mode = 'false'\\n)\"]]"; + let trace_meta_table_sql = "[[\"mytable_services\",\"CREATE TABLE IF NOT EXISTS \\\"mytable_services\\\" (\\n \\\"timestamp\\\" TIMESTAMP(9) NOT NULL,\\n \\\"service_name\\\" STRING NULL,\\n TIME INDEX (\\\"timestamp\\\"),\\n PRIMARY KEY (\\\"service_name\\\")\\n)\\n\\nENGINE=mito\\nWITH(\\n 'comment' = 'Created on insertion',\\n append_mode = 'false'\\n)\"]]"; validate_data( "trace_v1_create_meta_table", &client, From 4d7d4742c30d2448f177e8a07dbe3048bb050d28 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Fri, 5 Dec 2025 13:36:29 +0800 Subject: [PATCH 21/21] use try_as_raw_value Signed-off-by: Ruihang Xia --- src/common/meta/src/ddl/comment_on.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/meta/src/ddl/comment_on.rs b/src/common/meta/src/ddl/comment_on.rs index 9dd46d6da9fe..37b614ba5e2a 100644 --- a/src/common/meta/src/ddl/comment_on.rs +++ b/src/common/meta/src/ddl/comment_on.rs @@ -305,7 +305,7 @@ impl CommentOnProcedure { new_flow_info.comment = self.data.comment.clone().unwrap_or_default(); new_flow_info.updated_time = Utc::now(); - let raw_value = serde_json::to_vec(&new_flow_info).context(crate::error::SerdeJsonSnafu)?; + let raw_value = new_flow_info.try_as_raw_value()?; self.context .table_metadata_manager