Skip to content

Commit

Permalink
compatibility
Browse files Browse the repository at this point in the history
Signed-off-by: Zhenchi <[email protected]>
  • Loading branch information
zhongzc committed Nov 4, 2024
1 parent a01ed57 commit 78d9243
Show file tree
Hide file tree
Showing 11 changed files with 93 additions and 100 deletions.
24 changes: 5 additions & 19 deletions src/api/src/v1/column_def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,11 @@ pub fn options_from_column_schema(column_schema: &ColumnSchema) -> Option<Column
.options
.insert(FULLTEXT_GRPC_KEY.to_string(), fulltext.clone());
}

let inverted_index = column_schema
.metadata()
.get(INVERTED_INDEX_KEY)
.cloned()
.unwrap_or_else(|| false.to_string());
options
.options
.insert(INVERTED_INDEX_GRPC_KEY.to_string(), inverted_index);
if let Some(inverted_index) = column_schema.metadata().get(INVERTED_INDEX_KEY) {
options
.options
.insert(INVERTED_INDEX_GRPC_KEY.to_string(), inverted_index.clone());
}

(!options.options.is_empty()).then_some(options)
}
Expand All @@ -108,16 +104,6 @@ pub fn options_from_fulltext(fulltext: &FulltextOptions) -> Result<Option<Column
Ok((!options.options.is_empty()).then_some(options))
}

/// Tries to set the inverted index option in the given `ColumnOptions`.
pub fn set_inverted_index_if_absent(options: &mut ColumnOptions, is_primary_key: bool) {
if !options.options.contains_key(INVERTED_INDEX_GRPC_KEY) {
options.options.insert(
INVERTED_INDEX_GRPC_KEY.to_string(),
is_primary_key.to_string(),
);
}
}

#[cfg(test)]
mod tests {

Expand Down
18 changes: 5 additions & 13 deletions src/common/grpc-expr/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@

use std::collections::HashSet;

use api::v1::column_def::contains_fulltext;
use api::v1::{
column_def, AddColumn, AddColumns, Column, ColumnDataType, ColumnDataTypeExtension, ColumnDef,
AddColumn, AddColumns, Column, ColumnDataType, ColumnDataTypeExtension, ColumnDef,
ColumnOptions, ColumnSchema, CreateTableExpr, SemanticType,
};
use datatypes::schema::Schema;
Expand Down Expand Up @@ -106,12 +107,8 @@ pub fn build_create_table_expr(
} in column_exprs
{
let mut is_nullable = true;
let mut is_primary_key = false;
match semantic_type {
v if v == SemanticType::Tag as i32 => {
primary_keys.push(column_name.to_string());
is_primary_key = true;
}
v if v == SemanticType::Tag as i32 => primary_keys.push(column_name.to_string()),
v if v == SemanticType::Timestamp as i32 => {
ensure!(
time_index.is_none(),
Expand All @@ -127,16 +124,11 @@ pub fn build_create_table_expr(
_ => {}
}

let mut options = options.clone();
let opt = options.get_or_insert_with(ColumnOptions::default);
// Automatically set inverted index for primary key columns if table is created on insertions.
column_def::set_inverted_index_if_absent(opt, is_primary_key);

let column_type =
ColumnDataType::try_from(datatype).context(UnknownColumnDataTypeSnafu { datatype })?;

ensure!(
!column_def::contains_fulltext(&options) || column_type == ColumnDataType::String,
!contains_fulltext(options) || column_type == ColumnDataType::String,
InvalidFulltextColumnTypeSnafu {
column_name,
column_type,
Expand All @@ -151,7 +143,7 @@ pub fn build_create_table_expr(
semantic_type,
comment: String::new(),
datatype_extension: datatype_extension.clone(),
options,
options: options.clone(),
};
column_defs.push(column_def);
}
Expand Down
2 changes: 1 addition & 1 deletion src/mito2/src/sst/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ impl<'a> IndexerBuilder<'a> {
}

fn inverted_indexed_column_ids(&self) -> HashSet<ColumnId> {
// For compatibility
// Default to use primary key columns as inverted index columns.
let pk_as_inverted_index = !self
.metadata
.column_metadatas
Expand Down
36 changes: 19 additions & 17 deletions src/operator/src/expr_factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,12 @@ pub(crate) async fn create_external_expr(
// expanded form
let time_index = find_time_index(&create.constraints)?;
let primary_keys = find_primary_keys(&create.columns, &create.constraints)?;
let inverted_index_cols =
find_inverted_index_cols(&primary_keys, &create.columns, &create.constraints)?;
let inverted_index_cols = find_inverted_index_cols(&create.columns, &create.constraints)?;
let column_schemas = columns_to_column_schemas(
&create.columns,
&time_index,
&inverted_index_cols,
&primary_keys,
Some(&query_ctx.timezone()),
)?;
(time_index, primary_keys, column_schemas)
Expand Down Expand Up @@ -192,8 +192,7 @@ pub fn create_to_expr(
);

let primary_keys = find_primary_keys(&create.columns, &create.constraints)?;
let inverted_index_cols =
find_inverted_index_cols(&primary_keys, &create.columns, &create.constraints)?;
let inverted_index_cols = find_inverted_index_cols(&create.columns, &create.constraints)?;

let expr = CreateTableExpr {
catalog_name,
Expand Down Expand Up @@ -352,13 +351,11 @@ pub fn find_time_index(constraints: &[TableConstraint]) -> Result<String> {
}

/// Finds the inverted index columns from the constraints. If no inverted index
/// columns are provided in the constraints, use the primary keys as the inverted
/// index columns.
/// columns are provided in the constraints, return `None`.
fn find_inverted_index_cols(
primary_keys: &[String],
columns: &[SqlColumn],
constraints: &[TableConstraint],
) -> Result<Vec<String>> {
) -> Result<Option<Vec<String>>> {
let inverted_index_cols = constraints.iter().find_map(|constraint| {
if let TableConstraint::InvertedIndex { columns } = constraint {
Some(
Expand All @@ -373,7 +370,7 @@ fn find_inverted_index_cols(
});

let Some(inverted_index_cols) = inverted_index_cols else {
return Ok(primary_keys.to_owned());
return Ok(None);
};

for col in &inverted_index_cols {
Expand All @@ -385,33 +382,38 @@ fn find_inverted_index_cols(
}
}

Ok(inverted_index_cols)
Ok(Some(inverted_index_cols))
}

fn columns_to_expr(
column_defs: &[SqlColumn],
time_index: &str,
primary_keys: &[String],
invereted_index_cols: &[String],
invereted_index_cols: &Option<Vec<String>>,
timezone: Option<&Timezone>,
) -> Result<Vec<api::v1::ColumnDef>> {
let column_schemas =
columns_to_column_schemas(column_defs, time_index, invereted_index_cols, timezone)?;
let column_schemas = columns_to_column_schemas(
column_defs,
time_index,
invereted_index_cols,
primary_keys,
timezone,
)?;
column_schemas_to_defs(column_schemas, primary_keys)
}

fn columns_to_column_schemas(
columns: &[SqlColumn],
time_index: &str,
invereted_index_cols: &[String],
invereted_index_cols: &Option<Vec<String>>,
primary_keys: &[String],
timezone: Option<&Timezone>,
) -> Result<Vec<ColumnSchema>> {
columns
.iter()
.map(|c| {
let is_time_index = c.name().to_string() == time_index;
let with_inverted_index = invereted_index_cols.contains(&c.name().value);
column_to_schema(c, is_time_index, with_inverted_index, timezone).context(ParseSqlSnafu)
column_to_schema(c, time_index, invereted_index_cols, primary_keys, timezone)
.context(ParseSqlSnafu)
})
.collect::<Result<Vec<ColumnSchema>>>()
}
Expand Down
17 changes: 10 additions & 7 deletions src/query/src/sql/show_create_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,15 +148,18 @@ fn create_table_constraints(
.collect();
constraints.push(TableConstraint::PrimaryKey { columns });
}
let inverted_index_cols = schema

let inverted_index_set = schema
.column_schemas()
.iter()
.filter(|c| c.is_inverted_indexed())
.map(|c| Ident::with_quote(quote_style, &c.name))
.collect::<Vec<_>>();
if !inverted_index_cols.is_empty() || !table_meta.primary_key_indices.is_empty() {
// Do not show inverted index constraint if there are no inverted index columns
// and no primary key columns.
.any(|c| c.has_inverted_index_key());
if inverted_index_set {
let inverted_index_cols = schema
.column_schemas()
.iter()
.filter(|c| c.is_inverted_indexed())
.map(|c| Ident::with_quote(quote_style, &c.name))
.collect::<Vec<_>>();
constraints.push(TableConstraint::InvertedIndex {
columns: inverted_index_cols,
});
Expand Down
37 changes: 27 additions & 10 deletions src/sql/src/statements.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,13 @@ pub fn has_primary_key_option(column_def: &ColumnDef) -> bool {
/// Create a `ColumnSchema` from `Column`.
pub fn column_to_schema(
column: &Column,
is_time_index: bool,
with_inverted_index: bool,
time_index: &str,
invereted_index_cols: &Option<Vec<String>>,
primary_keys: &[String],
timezone: Option<&Timezone>,
) -> Result<ColumnSchema> {
let is_time_index = column.name().value == time_index;

let is_nullable = column
.options()
.iter()
Expand All @@ -470,12 +473,25 @@ pub fn column_to_schema(

let mut column_schema = ColumnSchema::new(name, data_type, is_nullable)
.with_time_index(is_time_index)
.set_inverted_index(with_inverted_index)
.with_default_constraint(default_constraint)
.context(error::InvalidDefaultSnafu {
column: &column.name().value,
})?;

// To keep compatibility,
// 1. if inverted index columns is not set, leave it empty meaning primary key columns will be used
// 2. if inverted index columns is set and non-empty, set selected columns to be inverted indexed
// 3. if inverted index columns is set and empty, set primary key columns to be non-inverted indexed explicitly
if let Some(inverted_index_cols) = invereted_index_cols {
if inverted_index_cols.is_empty() {
if primary_keys.contains(&column.name().value) {
column_schema = column_schema.set_inverted_index(false);
}
} else if inverted_index_cols.contains(&column.name().value) {
column_schema = column_schema.set_inverted_index(true);
}
}

if let Some(ColumnOption::Comment(c)) = column.options().iter().find_map(|o| {
if matches!(o.option, ColumnOption::Comment(_)) {
Some(&o.option)
Expand Down Expand Up @@ -1339,7 +1355,7 @@ mod tests {
extensions: ColumnExtensions::default(),
};

let column_schema = column_to_schema(&column_def, false, false, None).unwrap();
let column_schema = column_to_schema(&column_def, "ts", &None, &[], None).unwrap();

assert_eq!("col", column_schema.name);
assert_eq!(
Expand All @@ -1349,7 +1365,7 @@ mod tests {
assert!(column_schema.is_nullable());
assert!(!column_schema.is_time_index());

let column_schema = column_to_schema(&column_def, true, false, None).unwrap();
let column_schema = column_to_schema(&column_def, "col", &None, &[], None).unwrap();

assert_eq!("col", column_schema.name);
assert_eq!(
Expand Down Expand Up @@ -1378,7 +1394,7 @@ mod tests {
extensions: ColumnExtensions::default(),
};

let column_schema = column_to_schema(&column_def, false, false, None).unwrap();
let column_schema = column_to_schema(&column_def, "ts", &None, &[], None).unwrap();

assert_eq!("col2", column_schema.name);
assert_eq!(ConcreteDataType::string_datatype(), column_schema.data_type);
Expand Down Expand Up @@ -1412,8 +1428,9 @@ mod tests {

let column_schema = column_to_schema(
&column,
false,
false,
"ts",
&None,
&[],
Some(&Timezone::from_tz_string("Asia/Shanghai").unwrap()),
)
.unwrap();
Expand All @@ -1432,7 +1449,7 @@ mod tests {
);

// without timezone
let column_schema = column_to_schema(&column, false, false, None).unwrap();
let column_schema = column_to_schema(&column, "ts", &None, &[], None).unwrap();

assert_eq!("col", column_schema.name);
assert_eq!(
Expand Down Expand Up @@ -1474,7 +1491,7 @@ mod tests {
},
};

let column_schema = column_to_schema(&column, false, false, None).unwrap();
let column_schema = column_to_schema(&column, "ts", &None, &[], None).unwrap();
assert_eq!("col", column_schema.name);
assert_eq!(ConcreteDataType::string_datatype(), column_schema.data_type);
let fulltext_options = column_schema.fulltext_options().unwrap().unwrap();
Expand Down
3 changes: 1 addition & 2 deletions tests-integration/tests/grpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,7 @@ async fn insert_with_hints_and_assert(db: &Database) {
| | \"memory\" DOUBLE NULL, |
| | \"ts\" TIMESTAMP(3) NOT NULL, |
| | TIME INDEX (\"ts\"), |
| | PRIMARY KEY (\"host\"), |
| | INVERTED INDEX (\"host\") |
| | PRIMARY KEY (\"host\") |
| | ) |
| | |
| | ENGINE=mito |
Expand Down
3 changes: 1 addition & 2 deletions tests/cases/standalone/common/create/create.result
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,7 @@ SHOW CREATE TABLE monitor;
| | "cpu" DOUBLE NULL DEFAULT 0, |
| | "memory" DOUBLE NULL, |
| | TIME INDEX ("ts"), |
| | PRIMARY KEY ("host"), |
| | INVERTED INDEX ("host") |
| | PRIMARY KEY ("host") |
| | ) |
| | |
| | ENGINE=mito |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ SHOW CREATE TABLE data_types;
| | "ts6" TIMESTAMP(6) NULL, |
| | "ts9" TIMESTAMP(9) NOT NULL DEFAULT current_timestamp(), |
| | TIME INDEX ("ts9"), |
| | PRIMARY KEY ("s"), |
| | INVERTED INDEX ("s") |
| | PRIMARY KEY ("s") |
| | ) |
| | |
| | ENGINE=mito |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ SELECT SUM(region_rows), SUM(disk_size), SUM(sst_size), SUM(index_size)
+-------------------------------------------------------+-----------------------------------------------------+----------------------------------------------------+------------------------------------------------------+
| SUM(information_schema.region_statistics.region_rows) | SUM(information_schema.region_statistics.disk_size) | SUM(information_schema.region_statistics.sst_size) | SUM(information_schema.region_statistics.index_size) |
+-------------------------------------------------------+-----------------------------------------------------+----------------------------------------------------+------------------------------------------------------+
| 3 | 2442 | 0 | 0 |
| 3 | 2145 | 0 | 0 |
+-------------------------------------------------------+-----------------------------------------------------+----------------------------------------------------+------------------------------------------------------+

DROP TABLE test;
Expand Down
Loading

0 comments on commit 78d9243

Please sign in to comment.