From ad7a9c3518926a623e23966838ab7c6ede8df787 Mon Sep 17 00:00:00 2001 From: fys <40801205+fengys1996@users.noreply.github.com> Date: Mon, 12 Oct 2026 10:03:07 +0800 Subject: [PATCH] fix(json2): support settings in ALTER TABLE ADD COLUMN [Backport release/v1.3] (#9513) * fix(json2): preserve metadata when adding columns via alter table (#8696) * fix(json2): support adding columns via alter table Signed-off-by: fys * fix(json2): scope missing-column null handling to JSON2 --------- Signed-off-by: fys (cherry picked from commit c46fbde9acd6f87bfcb7d0fa12d79e80d871b17b) Signed-off-by: fys * fix(json2): support settings in ALTER TABLE ADD COLUMN (#9472) * fix(json2): support settings in ALTER TABLE ADD COLUMN Signed-off-by: fys * refactor(sql): reuse Column for ADD COLUMN definitions Signed-off-by: fys * chore: better method name * refactor(sql): share column parsing between CREATE TABLE and ADD COLUMN Signed-off-by: fys * chore: simplify tests Signed-off-by: fys * fix(sql): accept trailing commas in ALTER TABLE ADD COLUMN Signed-off-by: fys * fix(sql): preserve ALTER TABLE ADD COLUMN compatibility Signed-off-by: fys --------- Signed-off-by: fys (cherry picked from commit fe8a647bc8f87897520f10ed2c2bcbb8629642ff) Signed-off-by: fys --------- Signed-off-by: fys --- Cargo.lock | 1 + src/mito2/src/read/compat.rs | 44 ++-- src/operator/src/expr_helper.rs | 64 +++++- src/sql/Cargo.toml | 1 + src/sql/src/parsers/alter_parser.rs | 211 +++++++++++++----- src/sql/src/parsers/create_parser.rs | 43 ++-- src/sql/src/statements.rs | 179 +++++++++------ src/sql/src/statements/alter.rs | 12 +- src/sql/src/statements/create.rs | 7 +- .../src/statements/transform/type_alias.rs | 2 +- .../standalone/common/types/json/json2.result | 62 +++++ .../standalone/common/types/json/json2.sql | 30 +++ .../common/types/json/json2_alter.result | 87 ++++++++ .../common/types/json/json2_alter.sql | 27 +++ 14 files changed, 599 insertions(+), 171 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 602f1f60d87..56115b27006 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14039,6 +14039,7 @@ version = "1.3.0-beta.1" dependencies = [ "api", "arrow-buffer 59.2.0", + "arrow-schema 59.2.0", "chrono", "common-base", "common-catalog", diff --git a/src/mito2/src/read/compat.rs b/src/mito2/src/read/compat.rs index 0e566b7e078..acc05f0e9d2 100644 --- a/src/mito2/src/read/compat.rs +++ b/src/mito2/src/read/compat.rs @@ -19,7 +19,7 @@ use std::sync::Arc; use api::v1::SemanticType; use datatypes::arrow::array::{ - Array, ArrayRef, BinaryArray, BinaryBuilder, DictionaryArray, UInt32Array, + Array, ArrayRef, BinaryArray, BinaryBuilder, DictionaryArray, UInt32Array, new_null_array, }; use datatypes::arrow::compute::{TakeOptions, take}; use datatypes::arrow::datatypes::{FieldRef, Schema, SchemaRef}; @@ -27,7 +27,7 @@ use datatypes::arrow::record_batch::RecordBatch; use datatypes::data_type::ConcreteDataType; use datatypes::prelude::DataType; use datatypes::value::Value; -use datatypes::vectors::VectorRef; +use datatypes::vectors::{Helper, VectorRef}; use mito_codec::row_converter::{ CompositeValues, PrimaryKeyCodec, SortField, build_primary_key_codec, build_primary_key_codec_with_fields, @@ -192,20 +192,32 @@ impl FlatCompatBatch { }); } else { // Create a default vector with 1 element for that column. - let default_vector = expect_column - .column_schema - .create_default_vector(1) - .context(CreateDefaultSnafu { - region_id: expect_metadata.region_id, - column: &expect_column.column_schema.name, - })? - .with_context(|| CompatReaderSnafu { - region_id: expect_metadata.region_id, - reason: format!( - "column {} does not have a default value to read", - expect_column.column_schema.name - ), - })?; + let default_vector = if expect_data_type.is_json2() { + // JSON2 columns do not support defaults, so missing columns use NULL. + // `expect_data_type` may be a query-concretized JSON2 type, + // while region metadata still carries its unconcretized type. + // The default vector must match the target Arrow field above. + Helper::try_into_vector(new_null_array(&expect_data_type.as_arrow_type(), 1)) + .context(CreateDefaultSnafu { + region_id: expect_metadata.region_id, + column: &expect_column.column_schema.name, + })? + } else { + expect_column + .column_schema + .create_default_vector(1) + .context(CreateDefaultSnafu { + region_id: expect_metadata.region_id, + column: &expect_column.column_schema.name, + })? + .with_context(|| CompatReaderSnafu { + region_id: expect_metadata.region_id, + reason: format!( + "column {} does not have a default value to read", + expect_column.column_schema.name + ), + })? + }; index_or_defaults.push(IndexOrDefault::DefaultValue { default_vector, semantic_type: expect_column.semantic_type, diff --git a/src/operator/src/expr_helper.rs b/src/operator/src/expr_helper.rs index 1d06c24ff7f..d69277a1c90 100644 --- a/src/operator/src/expr_helper.rs +++ b/src/operator/src/expr_helper.rs @@ -68,8 +68,8 @@ use sql::statements::create::{ CreateView, TableConstraint, }; use sql::statements::{ - OptionMap, column_to_schema, concrete_data_type_to_sql_data_type, - sql_column_def_to_grpc_column_def, sql_data_type_to_concrete_data_type, value_to_sql_value, + OptionMap, column_to_schema, concrete_data_type_to_sql_data_type, sql_col_to_grpc_col_def, + sql_data_type_to_concrete_data_type, value_to_sql_value, }; use sql::util::extract_tables_from_query; use store_api::mito_engine_options::{COMPACTION_OVERRIDE, COMPACTION_TYPE}; @@ -828,12 +828,10 @@ pub(crate) fn to_alter_table_expr( add_columns: add_columns .into_iter() .map(|add_column| { - let column_def = sql_column_def_to_grpc_column_def( - &add_column.column_def, - Some(&query_ctx.timezone()), - ) - .map_err(BoxedError::new) - .context(ExternalSnafu)?; + let column_def = + sql_col_to_grpc_col_def(&add_column.column, Some(&query_ctx.timezone())) + .map_err(BoxedError::new) + .context(ExternalSnafu)?; if is_interval_type(&column_def.data_type()) { return NotSupportedSnafu { feat: "Add column with interval type", @@ -1192,6 +1190,7 @@ mod tests { use std::collections::HashMap; use api::v1::{SetDatabaseOptions, UnsetDatabaseOptions}; + use datatypes::extension::json::is_json2_extension_type; use datatypes::value::Value; use session::context::{QueryContext, QueryContextBuilder}; use sql::dialect::GreptimeDbDialect; @@ -1201,6 +1200,55 @@ mod tests { use super::*; + #[test] + fn test_to_alter_add_json2_column_preserves_extension() { + let sql = "ALTER TABLE monitor ADD COLUMN payload JSON2;"; + let stmt = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap() + .pop() + .unwrap(); + let Statement::AlterTable(alter_table) = stmt else { + unreachable!() + }; + + let expr = to_alter_table_expr(alter_table, &QueryContext::arc()).unwrap(); + let AlterTableKind::AddColumns(AddColumns { add_columns, .. }) = expr.kind.unwrap() else { + unreachable!() + }; + let column_def = add_columns[0].column_def.as_ref().unwrap(); + let column_schema = try_as_column_schema(column_def).unwrap(); + let schema = Schema::new(vec![column_schema]); + + assert!(is_json2_extension_type(&schema.arrow_schema().fields()[0])); + } + + #[test] + fn test_to_alter_add_json2_column_preserves_settings() { + let sql = "ALTER TABLE monitor ADD COLUMN payload JSON2(max_auto_expanded_paths = 0, service STRING, nested.value BIGINT)"; + let stmt = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap() + .pop() + .unwrap(); + let Statement::AlterTable(alter_table) = stmt else { + unreachable!() + }; + let expr = to_alter_table_expr(alter_table, &QueryContext::arc()).unwrap(); + let AlterTableKind::AddColumns(AddColumns { add_columns, .. }) = expr.kind.unwrap() else { + unreachable!() + }; + let column_schema = + try_as_column_schema(add_columns[0].column_def.as_ref().unwrap()).unwrap(); + let metadata: datatypes::extension::json::JsonMetadata = + serde_json::from_str(&column_schema.metadata()["ARROW:extension:metadata"]).unwrap(); + let settings = metadata.json_settings(); + assert_eq!(Some(0), settings.max_auto_expanded_paths()); + assert_eq!(2, settings.type_hints().len()); + assert_eq!(vec!["service"], settings.type_hints()[0].path); + assert_eq!(vec!["nested", "value"], settings.type_hints()[1].path); + } + #[test] fn test_create_flow_tql_expr() { let sql = r#" diff --git a/src/sql/Cargo.toml b/src/sql/Cargo.toml index 685d96336b1..de972bec30e 100644 --- a/src/sql/Cargo.toml +++ b/src/sql/Cargo.toml @@ -13,6 +13,7 @@ enterprise = [] [dependencies] api.workspace = true arrow-buffer.workspace = true +arrow-schema.workspace = true chrono.workspace = true common-base.workspace = true common-catalog.workspace = true diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 3e5c27fda73..62c417d2537 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -26,7 +26,7 @@ use sqlparser::parser::IsOptional::Mandatory; use sqlparser::parser::{Parser, ParserError}; use sqlparser::tokenizer::{Token, TokenWithSpan}; -use crate::ast::ObjectNamePartExt; +use crate::ast::{ColumnOption, ObjectNamePartExt}; use crate::error::{self, InvalidColumnOptionSnafu, Result, SetFulltextOptionSnafu}; use crate::parser::ParserContext; use crate::parsers::create_parser::{INVERTED, parse_json2_type_and_options}; @@ -379,14 +379,67 @@ impl ParserContext<'_> { Ok(AlterTableOperation::AddConstraint(constraint)) } else { self.parser.prev_token(); - let add_columns = self - .parser - .parse_comma_separated(parse_add_columns) - .context(error::SyntaxSnafu)?; + let mut add_columns = vec![self.parse_add_column()?]; + while self.parser.consume_token(&Token::Comma) { + if matches!( + self.parser.peek_token().token, + Token::SemiColon | Token::EOF + ) || self.matches_keyword(Keyword::WITH) + { + break; + } + add_columns.push(self.parse_add_column()?); + } Ok(AlterTableOperation::AddColumns { add_columns }) } } + fn parse_add_column(&mut self) -> Result { + self.parser + .expect_keyword(Keyword::ADD) + .context(error::SyntaxSnafu)?; + let _ = self.parser.parse_keyword(Keyword::COLUMN); + let add_if_not_exists = + self.parser + .parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]); + let column = self.parse_column_def(true)?; + + // These attributes need ALTER-specific execution support. + ensure!( + column.extensions.fulltext_index_options.is_none() + && column.extensions.skipping_index_options.is_none() + && column.extensions.inverted_index_options.is_none() + && !column + .options() + .iter() + .any(|option| matches!(option.option, ColumnOption::DialectSpecific(_))), + InvalidColumnOptionSnafu { + name: column.name().to_string(), + msg: "ADD COLUMN does not support inline indexes or TIME INDEX", + } + ); + + let location = if self.parser.parse_keyword(Keyword::FIRST) { + Some(AddColumnLocation::First) + } else if matches!(self.parser.peek_token().token, Token::Word(word) if word.value.eq_ignore_ascii_case("AFTER")) + { + self.parser.next_token(); + let name = Self::canonicalize_identifier( + self.parser.parse_identifier().context(error::SyntaxSnafu)?, + ); + Some(AddColumnLocation::After { + column_name: name.value, + }) + } else { + None + }; + Ok(AddColumn { + column, + location, + add_if_not_exists, + }) + } + fn parse_alter_table_drop_default( &mut self, column_name: Ident, @@ -704,34 +757,6 @@ fn parse_string_options(parser: &mut Parser) -> std::result::Result<(String, Str Ok((name, value)) } -fn parse_add_columns(parser: &mut Parser) -> std::result::Result { - parser.expect_keyword(Keyword::ADD)?; - let _ = parser.parse_keyword(Keyword::COLUMN); - let add_if_not_exists = parser.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]); - let mut column_def = parser.parse_column_def()?; - column_def.name = ParserContext::canonicalize_identifier(column_def.name); - let location = if parser.parse_keyword(Keyword::FIRST) { - Some(AddColumnLocation::First) - } else if let Token::Word(word) = parser.peek_token().token { - if word.value.eq_ignore_ascii_case("AFTER") { - let _ = parser.next_token(); - let name = ParserContext::canonicalize_identifier(parser.parse_identifier()?); - Some(AddColumnLocation::After { - column_name: name.value, - }) - } else { - None - } - } else { - None - }; - Ok(AddColumn { - column_def, - location, - add_if_not_exists, - }) -} - /// Parses a comma separated list of string literals. fn parse_string_option_names(parser: &mut Parser) -> std::result::Result { parser.parse_literal_string() @@ -750,6 +775,75 @@ mod tests { use crate::dialect::GreptimeDbDialect; use crate::parser::ParseOptions; use crate::statements::alter::AlterDatabaseOperation; + use crate::statements::create::{Column, ColumnExtensions}; + + #[test] + fn test_add_column_matches_create_table() { + for (definition, location) in [ + ( + "payload JSON2(max_auto_expanded_paths = 100, service STRING) NOT NULL", + "AFTER ts", + ), + ("payload JSON2(service STRING) NULL", "FIRST"), + ("embedding VECTOR(3) NOT NULL", "AFTER ts"), + ("reading INT DEFAULT 42 COMMENT 'value'", ""), + ("\"select\" STRING NULL", ""), + ] { + let parse = |sql: &str| { + ParserContext::create_with_dialect( + sql, + &GreptimeDbDialect {}, + ParseOptions::default(), + ) + .unwrap() + .pop() + .unwrap() + }; + let Statement::CreateTable(create) = parse(&format!( + "CREATE TABLE t ({definition}, n INT, ts TIMESTAMP TIME INDEX)" + )) else { + unreachable!() + }; + let Statement::AlterTable(alter) = parse(&format!( + "ALTER TABLE t ADD COLUMN {definition} {location}, ADD COLUMN n INT" + )) else { + unreachable!() + }; + let AlterTableOperation::AddColumns { add_columns } = alter.alter_operation() else { + unreachable!() + }; + assert_eq!(add_columns.len(), 2); + for (expected, actual) in create.columns.iter().zip(add_columns) { + assert_eq!(expected, &actual.column); + } + let Statement::AlterTable(roundtrip) = parse(&alter.to_string()) else { + unreachable!() + }; + assert_eq!(alter, roundtrip); + } + } + + #[test] + fn test_add_column_rejects_unsupported_attributes() { + for definition in [ + "payload STRING FULLTEXT INDEX", + "payload STRING SKIPPING INDEX", + "payload STRING INVERTED INDEX", + "ts TIMESTAMP TIME INDEX", + ] { + let err = ParserContext::create_with_dialect( + &format!("ALTER TABLE t ADD COLUMN {definition}"), + &GreptimeDbDialect {}, + ParseOptions::default(), + ) + .unwrap_err(); + assert!( + err.to_string() + .contains("ADD COLUMN does not support inline indexes or TIME INDEX"), + "{err}" + ); + } + } #[test] fn test_parse_alter_database() { @@ -830,10 +924,14 @@ mod tests { match alter_operation { AlterTableOperation::AddColumns { add_columns } => { assert_eq!(add_columns.len(), 1); - assert_eq!("tagk_i", add_columns[0].column_def.name.value); - assert_eq!(DataType::String(None), add_columns[0].column_def.data_type); + assert_eq!("tagk_i", add_columns[0].column.column_def.name.value); + assert_eq!( + DataType::String(None), + add_columns[0].column.column_def.data_type + ); assert!( add_columns[0] + .column .column_def .options .iter() @@ -866,10 +964,14 @@ mod tests { assert_matches!(alter_operation, AlterTableOperation::AddColumns { .. }); match alter_operation { AlterTableOperation::AddColumns { add_columns } => { - assert_eq!("tagk_i", add_columns[0].column_def.name.value); - assert_eq!(DataType::String(None), add_columns[0].column_def.data_type); + assert_eq!("tagk_i", add_columns[0].column.column_def.name.value); + assert_eq!( + DataType::String(None), + add_columns[0].column.column_def.data_type + ); assert!( add_columns[0] + .column .column_def .options .iter() @@ -931,7 +1033,7 @@ mod tests { .zip(expecteds) .collect::, ColumnDef))>>() { - assert_eq!(add_column.column_def, expected.1); + assert_eq!(add_column.column.column_def, expected.1); assert_eq!(&expected.0, &add_column.location); } } @@ -962,28 +1064,37 @@ mod tests { AlterTableOperation::AddColumns { add_columns } => { let expected = [ AddColumn { - column_def: ColumnDef { - name: Ident::new("a"), - data_type: DataType::Integer(None), - options: vec![], + column: Column { + column_def: ColumnDef { + name: Ident::new("a"), + data_type: DataType::Integer(None), + options: vec![], + }, + extensions: ColumnExtensions::default(), }, location: None, add_if_not_exists: true, }, AddColumn { - column_def: ColumnDef { - name: Ident::new("b"), - data_type: DataType::String(None), - options: vec![], + column: Column { + column_def: ColumnDef { + name: Ident::new("b"), + data_type: DataType::String(None), + options: vec![], + }, + extensions: ColumnExtensions::default(), }, location: None, add_if_not_exists: false, }, AddColumn { - column_def: ColumnDef { - name: Ident::new("c"), - data_type: DataType::Int(None), - options: vec![], + column: Column { + column_def: ColumnDef { + name: Ident::new("c"), + data_type: DataType::Int(None), + options: vec![], + }, + extensions: ColumnExtensions::default(), }, location: None, add_if_not_exists: true, diff --git a/src/sql/src/parsers/create_parser.rs b/src/sql/src/parsers/create_parser.rs index d701415f3d2..dd698ddbe57 100644 --- a/src/sql/src/parsers/create_parser.rs +++ b/src/sql/src/parsers/create_parser.rs @@ -41,8 +41,8 @@ use table::requests::{validate_database_option, validate_database_option_value}; use crate::ast::{ColumnDef, Ident, ObjectNamePartExt}; use crate::error::{ self, InvalidColumnOptionSnafu, InvalidDatabaseOptionSnafu, InvalidDatabaseOptionValueSnafu, - InvalidFlowQuerySnafu, InvalidIntervalSnafu, InvalidSqlSnafu, InvalidTimeIndexSnafu, - MissingTimeIndexSnafu, Result, SyntaxSnafu, UnexpectedSnafu, UnsupportedSnafu, + InvalidFlowQuerySnafu, InvalidIntervalSnafu, InvalidTimeIndexSnafu, MissingTimeIndexSnafu, + Result, SyntaxSnafu, UnexpectedSnafu, UnsupportedSnafu, }; use crate::parser::{FLOW, ParserContext}; use crate::parsers::tql_parser; @@ -685,7 +685,7 @@ impl<'a> ParserContext<'a> { columns: &mut Vec, constraints: &mut Vec, ) -> Result<()> { - let mut column = self.parse_column_def()?; + let mut column = self.parse_column_def(false)?; let mut time_index_opt_idx = None; for (index, opt) in column.options().iter().enumerate() { @@ -775,22 +775,16 @@ impl<'a> ParserContext<'a> { Ok(name) } - pub fn parse_column_def(&mut self) -> Result { - let name = self.parse_column_name().context(SyntaxSnafu)?; + /// Parses a column definition, optionally allowing unquoted keyword names + /// for compatibility with ALTER TABLE ADD COLUMN. + pub fn parse_column_def(&mut self, allow_keyword_name: bool) -> Result { + let name = if allow_keyword_name { + self.parser.parse_identifier().context(SyntaxSnafu)? + } else { + self.parse_column_name().context(SyntaxSnafu)? + }; let parser = &mut self.parser; - ensure!( - !(name.quote_style.is_none() && - // "ALL_KEYWORDS" are sorted. - ALL_KEYWORDS.binary_search(&name.value.to_uppercase().as_str()).is_ok()), - InvalidSqlSnafu { - msg: format!( - "Cannot use keyword '{}' as column name. Hint: add quotes to the name.", - &name.value - ), - } - ); - let mut extensions = ColumnExtensions::default(); let data_type = @@ -2924,11 +2918,16 @@ ENGINE=mito"; #[test] fn test_invalid_column_name() { - let sql = "create table foo(user string, i timestamp time index)"; - let result = - ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()); - let err = result.unwrap_err().output_msg(); - assert!(err.contains("Cannot use keyword 'user' as column name")); + for name in ["user", "at", "value", "select"] { + let sql = format!("create table foo({name} string, i timestamp time index)"); + let result = ParserContext::create_with_dialect( + &sql, + &GreptimeDbDialect {}, + ParseOptions::default(), + ); + let err = result.unwrap_err().output_msg(); + assert!(err.contains(&format!("Cannot use keyword '{name}' as column name"))); + } // If column name is quoted, it's valid even same with keyword. let sql = r#" diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 2ea716678c7..c2c97c34567 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -36,7 +36,10 @@ pub mod truncate; use std::sync::Arc; use api::helper::ColumnDataTypeWrapper; -use api::v1::SemanticType; +use api::v1::{ColumnOptions, SemanticType}; +use arrow_schema::extension::{ + EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY, ExtensionType, +}; use common_sql::default_constraint::parse_column_default_constraint; use common_time::timezone::Timezone; use datatypes::extension::json::{Json2ExtensionType, JsonMetadata}; @@ -148,32 +151,34 @@ pub fn column_to_schema( column_schema.set_inverted_index(column.extensions.inverted_index_options.is_some()); - let is_json2_column = if let SqlDataType::Custom(object_name, _) = column.data_type() { - object_name - .0 - .first() - .map(|x| x.to_string_unquoted().eq_ignore_ascii_case(JSON2_TYPE_NAME)) - .unwrap_or_default() - } else { - false - }; - if is_json2_column { - let settings = column - .extensions - .build_json_settings()? - .unwrap_or_else(JsonSettings::new_v2); - let extension = Json2ExtensionType::new(Arc::new(JsonMetadata::new(settings))); - column_schema.with_extension_type(&extension); - } + set_json2_extension(&mut column_schema, column)?; Ok(column_schema) } -/// Convert `ColumnDef` in sqlparser to `ColumnDef` in gRPC proto. -pub fn sql_column_def_to_grpc_column_def( - col: &ColumnDef, +/// Attaches extension metadata and settings to JSON2 columns. +fn set_json2_extension(column_schema: &mut ColumnSchema, column: &Column) -> Result<()> { + if !column_schema.data_type.is_json2() { + return Ok(()); + } + + let settings = column + .extensions + .build_json_settings()? + .unwrap_or_else(JsonSettings::new_v2); + if let Some(extension) = json2_extension(column.data_type(), settings) { + column_schema.with_extension_type(&extension); + } + + Ok(()) +} + +/// Converts a SQL [`Column`] to a `ColumnDef` in gRPC proto. +pub fn sql_col_to_grpc_col_def( + column: &Column, timezone: Option<&Timezone>, ) -> Result { + let col = &column.column_def; let name = col.name.value.clone(); let data_type = sql_data_type_to_concrete_data_type(&col.data_type)?; @@ -204,6 +209,24 @@ pub fn sql_column_def_to_grpc_column_def( SemanticType::Field }; + let settings = column + .extensions + .build_json_settings()? + .unwrap_or_else(JsonSettings::new_v2); + let options = json2_extension(&col.data_type, settings).map(|extension| { + let mut options = ColumnOptions::default(); + options.options.insert( + EXTENSION_TYPE_NAME_KEY.to_string(), + Json2ExtensionType::NAME.to_string(), + ); + if let Some(metadata) = extension.serialize_metadata() { + options + .options + .insert(EXTENSION_TYPE_METADATA_KEY.to_string(), metadata); + } + options + }); + Ok(api::v1::ColumnDef { name, data_type: datatype as i32, @@ -212,10 +235,26 @@ pub fn sql_column_def_to_grpc_column_def( semantic_type: semantic_type as _, comment: String::new(), datatype_extension: datatype_ext, - options: None, + options, }) } +fn json2_extension(data_type: &SqlDataType, settings: JsonSettings) -> Option { + let SqlDataType::Custom(name, _) = data_type else { + return None; + }; + if !name.0.first().is_some_and(|name| { + name.to_string_unquoted() + .eq_ignore_ascii_case(JSON2_TYPE_NAME) + }) { + return None; + } + + Some(Json2ExtensionType::new(Arc::new(JsonMetadata::new( + settings, + )))) +} + pub fn sql_data_type_to_concrete_data_type(data_type: &SqlDataType) -> Result { match data_type { SqlDataType::BigInt(_) | SqlDataType::Int64 => Ok(ConcreteDataType::int64_datatype()), @@ -453,15 +492,18 @@ mod tests { } #[test] - pub fn test_sql_column_def_to_grpc_column_def() { + pub fn test_sql_col_to_grpc_col_def() { // test basic - let column_def = ColumnDef { - name: "col".into(), - data_type: SqlDataType::Double(ExactNumberInfo::None), - options: vec![], + let column = Column { + column_def: ColumnDef { + name: "col".into(), + data_type: SqlDataType::Double(ExactNumberInfo::None), + options: vec![], + }, + extensions: ColumnExtensions::default(), }; - let grpc_column_def = sql_column_def_to_grpc_column_def(&column_def, None).unwrap(); + let grpc_column_def = sql_col_to_grpc_col_def(&column, None).unwrap(); assert_eq!("col", grpc_column_def.name); assert!(grpc_column_def.is_nullable); // nullable when options are empty @@ -470,56 +512,65 @@ mod tests { assert_eq!(grpc_column_def.semantic_type, SemanticType::Field as i32); // test not null - let column_def = ColumnDef { - name: "col".into(), - data_type: SqlDataType::Double(ExactNumberInfo::None), - options: vec![ColumnOptionDef { - name: None, - option: ColumnOption::NotNull, - }], + let column = Column { + column_def: ColumnDef { + name: "col".into(), + data_type: SqlDataType::Double(ExactNumberInfo::None), + options: vec![ColumnOptionDef { + name: None, + option: ColumnOption::NotNull, + }], + }, + extensions: ColumnExtensions::default(), }; - let grpc_column_def = sql_column_def_to_grpc_column_def(&column_def, None).unwrap(); + let grpc_column_def = sql_col_to_grpc_col_def(&column, None).unwrap(); assert!(!grpc_column_def.is_nullable); // test primary key - let column_def = ColumnDef { - name: "col".into(), - data_type: SqlDataType::Double(ExactNumberInfo::None), - options: vec![ColumnOptionDef { - name: None, - option: ColumnOption::PrimaryKey(PrimaryKeyConstraint { + let column = Column { + column_def: ColumnDef { + name: "col".into(), + data_type: SqlDataType::Double(ExactNumberInfo::None), + options: vec![ColumnOptionDef { name: None, - index_name: None, - index_type: None, - columns: vec![], - index_options: vec![], - characteristics: None, - }), - }], + option: ColumnOption::PrimaryKey(PrimaryKeyConstraint { + name: None, + index_name: None, + index_type: None, + columns: vec![], + index_options: vec![], + characteristics: None, + }), + }], + }, + extensions: ColumnExtensions::default(), }; - let grpc_column_def = sql_column_def_to_grpc_column_def(&column_def, None).unwrap(); + let grpc_column_def = sql_col_to_grpc_col_def(&column, None).unwrap(); assert_eq!(grpc_column_def.semantic_type, SemanticType::Tag as i32); } #[test] - pub fn test_sql_column_def_to_grpc_column_def_with_timezone() { - let column_def = ColumnDef { - name: "col".into(), - // MILLISECOND - data_type: SqlDataType::Timestamp(Some(3), TimezoneInfo::None), - options: vec![ColumnOptionDef { - name: None, - option: ColumnOption::Default(Expr::Value( - SqlValue::SingleQuotedString("2024-01-30T00:01:01".to_string()).into(), - )), - }], + pub fn test_sql_col_to_grpc_col_def_with_timezone() { + let column = Column { + column_def: ColumnDef { + name: "col".into(), + // MILLISECOND + data_type: SqlDataType::Timestamp(Some(3), TimezoneInfo::None), + options: vec![ColumnOptionDef { + name: None, + option: ColumnOption::Default(Expr::Value( + SqlValue::SingleQuotedString("2024-01-30T00:01:01".to_string()).into(), + )), + }], + }, + extensions: ColumnExtensions::default(), }; // with timezone "Asia/Shanghai" - let grpc_column_def = sql_column_def_to_grpc_column_def( - &column_def, + let grpc_column_def = sql_col_to_grpc_col_def( + &column, Some(&Timezone::from_tz_string("Asia/Shanghai").unwrap()), ) .unwrap(); @@ -539,7 +590,7 @@ mod tests { ); // without timezone - let grpc_column_def = sql_column_def_to_grpc_column_def(&column_def, None).unwrap(); + let grpc_column_def = sql_col_to_grpc_col_def(&column, None).unwrap(); assert_eq!("col", grpc_column_def.name); assert!(grpc_column_def.is_nullable); // nullable when options are empty assert_eq!( diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 4fc45c2e69b..781c65ab528 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -22,11 +22,11 @@ use common_query::AddColumnLocation; use datatypes::schema::{FulltextOptions, SkippingIndexOptions}; use itertools::Itertools; use serde::Serialize; -use sqlparser::ast::{ColumnDef, DataType, Expr, Ident, ObjectName, TableConstraint}; +use sqlparser::ast::{DataType, Expr, Ident, ObjectName, TableConstraint}; use sqlparser_derive::{Visit, VisitMut}; use crate::statements::OptionMap; -use crate::statements::create::{Json2Options, Partitions}; +use crate::statements::create::{Column, Json2Options, Partitions}; #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] pub struct AlterTable { @@ -229,18 +229,18 @@ pub enum UnsetIndexOperation { #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] pub struct AddColumn { - pub column_def: ColumnDef, + pub column: Column, pub location: Option, pub add_if_not_exists: bool, } impl Display for AddColumn { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.column)?; if let Some(location) = &self.location { - write!(f, "{} {location}", self.column_def) - } else { - write!(f, "{}", self.column_def) + write!(f, " {location}")?; } + Ok(()) } } diff --git a/src/sql/src/statements/create.rs b/src/sql/src/statements/create.rs index 2cb78431969..b5bda7b291e 100644 --- a/src/sql/src/statements/create.rs +++ b/src/sql/src/statements/create.rs @@ -105,7 +105,7 @@ pub struct CreateTable { pub partitions: Option, } -/// Column definition in `CREATE TABLE` statement. +/// Column definition in `CREATE TABLE` and `ALTER TABLE ADD COLUMN`. #[derive(Debug, PartialEq, Eq, Clone, Visit, VisitMut, Serialize)] pub struct Column { /// `ColumnDef` from `sqlparser::ast` @@ -207,10 +207,9 @@ impl Display for Column { && let Some(dim) = vector_options.get(VECTOR_OPT_DIM) { write!(f, "{} VECTOR({})", self.column_def.name, dim)?; - return Ok(()); + } else { + write!(f, "{} {}", self.column_def.name, self.column_def.data_type)?; } - - write!(f, "{} {}", self.column_def.name, self.column_def.data_type)?; if let Some(options) = &self.extensions.json2_options { write!(f, "{options}")?; } diff --git a/src/sql/src/statements/transform/type_alias.rs b/src/sql/src/statements/transform/type_alias.rs index b2d1803fb37..90157901f3c 100644 --- a/src/sql/src/statements/transform/type_alias.rs +++ b/src/sql/src/statements/transform/type_alias.rs @@ -72,7 +72,7 @@ impl TransformRule for TypeAliasTransformRule { alter_table.alter_operation_mut() { for add_column in add_columns { - replace_type_alias(&mut add_column.column_def.data_type); + replace_type_alias(add_column.column.mut_data_type()); } } } diff --git a/tests/cases/standalone/common/types/json/json2.result b/tests/cases/standalone/common/types/json/json2.result index ce0c9748ce1..7114d9bafa1 100644 --- a/tests/cases/standalone/common/types/json/json2.result +++ b/tests/cases/standalone/common/types/json/json2.result @@ -513,3 +513,65 @@ drop table json2_finite_paths; Affected Rows: 0 +-- A newly added JSON2 column must not read data from a dropped column with the +-- same name in an older SST. Column identity is determined by column id, not +-- by the parquet root name. +create table json2_drop_add_same_name ( + ts timestamp time index, + b json2 +) with ( + 'append_mode' = 'true', + 'sst_format' = 'flat', +); + +Affected Rows: 0 + +insert into json2_drop_add_same_name +values (1, '{"a": "old"}'); + +Affected Rows: 1 + +admin flush_table('json2_drop_add_same_name'); + ++-----------------------------------------------+ +| ADMIN flush_table('json2_drop_add_same_name') | ++-----------------------------------------------+ +| 0 | ++-----------------------------------------------+ + +alter table json2_drop_add_same_name drop column b; + +Affected Rows: 0 + +alter table json2_drop_add_same_name add column b json2; + +Affected Rows: 0 + +insert into json2_drop_add_same_name +values (2, '{"a": "new"}'); + +Affected Rows: 1 + +admin flush_table('json2_drop_add_same_name'); + ++-----------------------------------------------+ +| ADMIN flush_table('json2_drop_add_same_name') | ++-----------------------------------------------+ +| 0 | ++-----------------------------------------------+ + +select ts, b.a +from json2_drop_add_same_name +order by ts; + ++-------------------------+--------------------------------------------------+ +| ts | json_get(json2_drop_add_same_name.b,Utf8("$.a")) | ++-------------------------+--------------------------------------------------+ +| 1970-01-01T00:00:00.001 | | +| 1970-01-01T00:00:00.002 | new | ++-------------------------+--------------------------------------------------+ + +drop table json2_drop_add_same_name; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/types/json/json2.sql b/tests/cases/standalone/common/types/json/json2.sql index 17248741c35..55642b7530c 100644 --- a/tests/cases/standalone/common/types/json/json2.sql +++ b/tests/cases/standalone/common/types/json/json2.sql @@ -157,3 +157,33 @@ from json2_finite_paths order by ts; drop table json2_finite_paths; + +-- A newly added JSON2 column must not read data from a dropped column with the +-- same name in an older SST. Column identity is determined by column id, not +-- by the parquet root name. +create table json2_drop_add_same_name ( + ts timestamp time index, + b json2 +) with ( + 'append_mode' = 'true', + 'sst_format' = 'flat', +); + +insert into json2_drop_add_same_name +values (1, '{"a": "old"}'); + +admin flush_table('json2_drop_add_same_name'); + +alter table json2_drop_add_same_name drop column b; +alter table json2_drop_add_same_name add column b json2; + +insert into json2_drop_add_same_name +values (2, '{"a": "new"}'); + +admin flush_table('json2_drop_add_same_name'); + +select ts, b.a +from json2_drop_add_same_name +order by ts; + +drop table json2_drop_add_same_name; diff --git a/tests/cases/standalone/common/types/json/json2_alter.result b/tests/cases/standalone/common/types/json/json2_alter.result index 153aa9e7902..055ea90346a 100644 --- a/tests/cases/standalone/common/types/json/json2_alter.result +++ b/tests/cases/standalone/common/types/json/json2_alter.result @@ -150,3 +150,90 @@ DROP TABLE application_logs; Affected Rows: 0 +create table json2_alter_add_settings ( + ts timestamp time index +) with ( + 'append_mode' = 'true' +); + +Affected Rows: 0 + +-- ADD COLUMN preserves JSON2 settings and type hints, including for existing rows. +insert into json2_alter_add_settings values (1); + +Affected Rows: 1 + +alter table json2_alter_add_settings add column j json2(max_auto_expanded_paths = 100); + +Affected Rows: 0 + +alter table json2_alter_add_settings add column k json2(service string); + +Affected Rows: 0 + +alter table json2_alter_add_settings add column m json2(max_auto_expanded_paths = 0, nested.value bigint); + +Affected Rows: 0 + +show create table json2_alter_add_settings; + ++--------------------------+---------------------------------------------------------+ +| Table | Create Table | ++--------------------------+---------------------------------------------------------+ +| json2_alter_add_settings | CREATE TABLE IF NOT EXISTS "json2_alter_add_settings" ( | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "j" JSON2( | +| | max_auto_expanded_paths = 100 | +| | ) NULL, | +| | "k" JSON2( | +| | max_auto_expanded_paths = 100, | +| | "service" STRING | +| | ) NULL, | +| | "m" JSON2( | +| | max_auto_expanded_paths = 0, | +| | "nested"."value" BIGINT | +| | ) NULL, | +| | TIME INDEX ("ts") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++--------------------------+---------------------------------------------------------+ + +insert into json2_alter_add_settings values + (2, '{"a":1}', '{"service":"api"}', '{"nested":{"value":42},"extra":true}'); + +Affected Rows: 1 + +select ts, j, k.service, m.nested.value, m from json2_alter_add_settings order by ts; + ++-------------------------+---------+-----------------------------------------------------------------------+-------------------------------------------------------------------------+--------------------------------------+ +| ts | j | json_get(json2_alter_add_settings.k,Utf8("$.service"),Utf8View(NULL)) | json_get(json2_alter_add_settings.m,Utf8("$.nested.value"),Int64(NULL)) | m | ++-------------------------+---------+-----------------------------------------------------------------------+-------------------------------------------------------------------------+--------------------------------------+ +| 1970-01-01T00:00:00.001 | | | | | +| 1970-01-01T00:00:00.002 | {"a":1} | api | 42 | {"extra":true,"nested":{"value":42}} | ++-------------------------+---------+-----------------------------------------------------------------------+-------------------------------------------------------------------------+--------------------------------------+ + +admin flush_table('json2_alter_add_settings'); + ++-----------------------------------------------+ +| ADMIN flush_table('json2_alter_add_settings') | ++-----------------------------------------------+ +| 0 | ++-----------------------------------------------+ + +select ts, j, k.service, m.nested.value, m from json2_alter_add_settings order by ts; + ++-------------------------+---------+-----------------------------------------------------------------------+-------------------------------------------------------------------------+--------------------------------------+ +| ts | j | json_get(json2_alter_add_settings.k,Utf8("$.service"),Utf8View(NULL)) | json_get(json2_alter_add_settings.m,Utf8("$.nested.value"),Int64(NULL)) | m | ++-------------------------+---------+-----------------------------------------------------------------------+-------------------------------------------------------------------------+--------------------------------------+ +| 1970-01-01T00:00:00.001 | | | | | +| 1970-01-01T00:00:00.002 | {"a":1} | api | 42 | {"extra":true,"nested":{"value":42}} | ++-------------------------+---------+-----------------------------------------------------------------------+-------------------------------------------------------------------------+--------------------------------------+ + +drop table json2_alter_add_settings; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/types/json/json2_alter.sql b/tests/cases/standalone/common/types/json/json2_alter.sql index 457943e93f6..83b514f6895 100644 --- a/tests/cases/standalone/common/types/json/json2_alter.sql +++ b/tests/cases/standalone/common/types/json/json2_alter.sql @@ -74,3 +74,30 @@ ORDER BY ts; ADMIN FLUSH_TABLE('application_logs'); DROP TABLE application_logs; + +create table json2_alter_add_settings ( + ts timestamp time index +) with ( + 'append_mode' = 'true' +); + +-- ADD COLUMN preserves JSON2 settings and type hints, including for existing rows. +insert into json2_alter_add_settings values (1); +alter table json2_alter_add_settings add column j json2(max_auto_expanded_paths = 100); + +alter table json2_alter_add_settings add column k json2(service string); + +alter table json2_alter_add_settings add column m json2(max_auto_expanded_paths = 0, nested.value bigint); + +show create table json2_alter_add_settings; + +insert into json2_alter_add_settings values + (2, '{"a":1}', '{"service":"api"}', '{"nested":{"value":42},"extra":true}'); + +select ts, j, k.service, m.nested.value, m from json2_alter_add_settings order by ts; + +admin flush_table('json2_alter_add_settings'); + +select ts, j, k.service, m.nested.value, m from json2_alter_add_settings order by ts; + +drop table json2_alter_add_settings;