Skip to content

Commit 12aee4e

Browse files
KKouldKKould
committed
chore: fix code reviews
Signed-off-by: Kould <kould2333@gmail.com>
1 parent a67e74e commit 12aee4e

File tree

12 files changed

+387
-319
lines changed

12 files changed

+387
-319
lines changed

‎src/query/catalog/src/plan/pushdown.rs

-3
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,6 @@ pub struct VirtualColumnField {
6363
pub cast_func_name: Option<String>,
6464
/// Virtual column data type.
6565
pub data_type: Box<TableDataType>,
66-
/// Is the virtual column is created,
67-
/// if not, reminder user to create it.
68-
pub is_created: bool,
6966
}
7067

7168
/// Information about prewhere optimization.

‎src/query/expression/src/schema.rs

-35
Original file line numberDiff line numberDiff line change
@@ -1284,41 +1284,6 @@ impl From<&TableDataType> for DataType {
12841284
}
12851285
}
12861286

1287-
impl From<&DataType> for Option<TableDataType> {
1288-
fn from(value: &DataType) -> Self {
1289-
let ty = match value {
1290-
DataType::Null => TableDataType::Null,
1291-
DataType::EmptyArray => TableDataType::EmptyArray,
1292-
DataType::EmptyMap => TableDataType::EmptyMap,
1293-
DataType::Boolean => TableDataType::Boolean,
1294-
DataType::Binary => TableDataType::Binary,
1295-
DataType::String => TableDataType::String,
1296-
DataType::Number(ty) => TableDataType::Number(*ty),
1297-
DataType::Decimal(ty) => TableDataType::Decimal(*ty),
1298-
DataType::Timestamp => TableDataType::Timestamp,
1299-
DataType::Date => TableDataType::Date,
1300-
DataType::Nullable(ty) => {
1301-
TableDataType::Nullable(Box::new(<&DataType as Into<Option<TableDataType>>>::into(
1302-
&**ty,
1303-
)?))
1304-
}
1305-
DataType::Array(ty) => TableDataType::Array(Box::new(<&DataType as Into<
1306-
Option<TableDataType>,
1307-
>>::into(&**ty)?)),
1308-
DataType::Map(ty) => TableDataType::Map(Box::new(<&DataType as Into<
1309-
Option<TableDataType>,
1310-
>>::into(&**ty)?)),
1311-
DataType::Bitmap => TableDataType::Bitmap,
1312-
DataType::Variant => TableDataType::Variant,
1313-
DataType::Geometry => TableDataType::Geometry,
1314-
DataType::Interval => TableDataType::Interval,
1315-
DataType::Geography => TableDataType::Geography,
1316-
DataType::Generic(_) | DataType::Tuple(_) => return None,
1317-
};
1318-
Some(ty)
1319-
}
1320-
}
1321-
13221287
impl TableDataType {
13231288
pub fn wrap_nullable(&self) -> Self {
13241289
match self {

‎src/query/sql/src/executor/physical_plans/physical_table_scan.rs

+7-8
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ use databend_common_expression::DataSchemaRef;
3737
use databend_common_expression::DataSchemaRefExt;
3838
use databend_common_expression::FieldIndex;
3939
use databend_common_expression::RemoteExpr;
40-
use databend_common_expression::TableDataType;
4140
use databend_common_expression::TableField;
4241
use databend_common_expression::TableSchema;
4342
use databend_common_expression::TableSchemaRef;
@@ -566,12 +565,13 @@ impl PhysicalPlanBuilder {
566565
{
567566
source_column_ids.insert(virtual_column.source_column_id);
568567
let cast_func_name =
569-
if virtual_column.data_type.remove_nullable() != TableDataType::Variant {
570-
let dest_type = DataType::from(&virtual_column.data_type.remove_nullable());
571-
get_simple_cast_function(true, &DataType::Variant, &dest_type)
572-
} else {
573-
None
574-
};
568+
virtual_column
569+
.cast_type
570+
.as_ref()
571+
.and_then(|(cast_ty, is_try)| {
572+
let dest_type = DataType::from(&cast_ty.remove_nullable());
573+
get_simple_cast_function(*is_try, &DataType::Variant, &dest_type)
574+
});
575575
let virtual_column_field = VirtualColumnField {
576576
source_column_id: virtual_column.source_column_id,
577577
source_name: virtual_column.source_column_name.clone(),
@@ -580,7 +580,6 @@ impl PhysicalPlanBuilder {
580580
key_paths: virtual_column.key_paths.clone(),
581581
cast_func_name,
582582
data_type: Box::new(virtual_column.data_type.clone()),
583-
is_created: virtual_column.is_created,
584583
};
585584
column_and_indices.push((virtual_column_field, *index));
586585
}

‎src/query/sql/src/planner/binder/bind_table_reference/bind_table.rs

+9-194
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,6 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use std::fmt::Write;
16-
use std::sync::Arc;
17-
1815
use databend_common_ast::ast::Identifier;
1916
use databend_common_ast::ast::SampleConfig;
2017
use databend_common_ast::ast::Statement;
@@ -24,27 +21,19 @@ use databend_common_ast::ast::WithOptions;
2421
use databend_common_ast::parser::parse_sql;
2522
use databend_common_ast::parser::tokenize_sql;
2623
use databend_common_ast::Span;
27-
use databend_common_catalog::table::Table;
2824
use databend_common_catalog::table::TimeNavigation;
2925
use databend_common_catalog::table_with_options::check_with_opt_valid;
3026
use databend_common_catalog::table_with_options::get_with_opt_consume;
3127
use databend_common_catalog::table_with_options::get_with_opt_max_batch_size;
3228
use databend_common_exception::ErrorCode;
3329
use databend_common_exception::Result;
34-
use databend_common_expression::types::DataType;
35-
use databend_common_expression::Scalar;
36-
use databend_common_expression::TableDataType;
3730
use databend_common_storages_view::view_table::QUERY;
3831
use databend_storages_common_table_meta::table::get_change_type;
3932

4033
use crate::binder::util::TableIdentifier;
4134
use crate::binder::Binder;
4235
use crate::optimizer::ir::SExpr;
4336
use crate::BindContext;
44-
use crate::ColumnBindingBuilder;
45-
use crate::ColumnEntry;
46-
use crate::IndexType;
47-
use crate::Visibility;
4837

4938
impl Binder {
5039
/// Bind a base table.
@@ -173,7 +162,9 @@ impl Binder {
173162
bind_context.planning_agg_index,
174163
false,
175164
None,
176-
);
165+
&mut bind_context.virtual_column_context,
166+
&mut bind_context.columns,
167+
)?;
177168
let (s_expr, mut bind_context) = self.bind_base_table(
178169
bind_context,
179170
database.as_str(),
@@ -258,7 +249,9 @@ impl Binder {
258249
false,
259250
false,
260251
None,
261-
);
252+
&mut bind_context.virtual_column_context,
253+
&mut bind_context.columns,
254+
)?;
262255
let (s_expr, mut new_bind_context) =
263256
self.bind_query(&mut new_bind_context, query)?;
264257
if let Some(alias) = alias {
@@ -290,8 +283,9 @@ impl Binder {
290283
bind_context.planning_agg_index,
291284
false,
292285
cte_suffix_name,
293-
);
294-
self.bind_table_virtual_column(bind_context, table_meta.clone(), table_index)?;
286+
&mut bind_context.virtual_column_context,
287+
&mut bind_context.columns,
288+
)?;
295289

296290
let (s_expr, mut bind_context) = self.bind_base_table(
297291
bind_context,
@@ -331,183 +325,4 @@ impl Binder {
331325
_ => Ok(()),
332326
}
333327
}
334-
335-
fn bind_table_virtual_column(
336-
&mut self,
337-
bind_context: &mut BindContext,
338-
table: Arc<dyn Table>,
339-
table_index: IndexType,
340-
) -> Result<()> {
341-
if !bind_context.virtual_column_context.allow_pushdown {
342-
return Ok(());
343-
}
344-
345-
// Ignore tables that do not support virtual columns
346-
if !table.support_virtual_columns() {
347-
return Ok(());
348-
}
349-
350-
let table_meta = &table.get_table_info().meta;
351-
let Some(ref virtual_schema) = table_meta.virtual_schema else {
352-
return Ok(());
353-
};
354-
355-
if !bind_context
356-
.virtual_column_context
357-
.table_indices
358-
.contains(&table_index)
359-
{
360-
bind_context
361-
.virtual_column_context
362-
.table_indices
363-
.insert(table_index);
364-
for virtual_field in virtual_schema.fields.iter() {
365-
let Some(base_column) = ({
366-
let guard = self.metadata.read();
367-
guard
368-
.columns_by_table_index(table_index)
369-
.iter()
370-
.find_map(|column| {
371-
if let ColumnEntry::BaseTableColumn(base_column) = column {
372-
if base_column.column_id == Some(virtual_field.source_column_id) {
373-
return Some(base_column.clone());
374-
}
375-
}
376-
None
377-
})
378-
}) else {
379-
continue;
380-
};
381-
382-
let name = format!("{}{}", base_column.column_name, virtual_field.name);
383-
let mut index = 0;
384-
// Check for duplicate virtual columns
385-
for table_column in self
386-
.metadata
387-
.read()
388-
.virtual_columns_by_table_index(table_index)
389-
{
390-
if table_column.name() == name {
391-
index = table_column.index();
392-
break;
393-
}
394-
}
395-
let column_id = virtual_field.column_id;
396-
397-
// It's not possible to determine the actual type based on the type of generated virtual columns,
398-
// as fields in non-leaf nodes are not generated with virtual columns,
399-
// and these ungenerated nodes may have inconsistent types.
400-
let table_data_type = TableDataType::Nullable(Box::new(TableDataType::Variant));
401-
let data_type = Box::new(DataType::from(&table_data_type));
402-
403-
if index == 0 {
404-
let is_created = true;
405-
406-
let path = Self::convert_array_index(&virtual_field.name)?;
407-
index = self.metadata.write().add_virtual_column(
408-
&base_column,
409-
column_id,
410-
name.clone(),
411-
table_data_type,
412-
Scalar::String(path),
413-
None,
414-
is_created,
415-
);
416-
}
417-
let virtual_column_binding =
418-
ColumnBindingBuilder::new(name, index, data_type, Visibility::InVisible)
419-
.table_index(Some(table_index))
420-
.build();
421-
bind_context.columns.push(virtual_column_binding);
422-
}
423-
}
424-
425-
Ok(())
426-
}
427-
428-
fn convert_array_index(input: &str) -> Result<String> {
429-
let mut chars = input.chars().peekable();
430-
let mut path = "{".to_string();
431-
432-
while chars.peek().is_some() {
433-
if chars.next() != Some('[') {
434-
return Err(ErrorCode::InvalidArgument("Expected '['".to_string()));
435-
}
436-
if let Some('\'') = chars.peek() {
437-
let _ = chars.next();
438-
path.push('"');
439-
440-
let mut is_empty = true;
441-
for c in chars.by_ref() {
442-
is_empty = false;
443-
if c == '\'' {
444-
break;
445-
}
446-
path.write_char(c)?;
447-
}
448-
if is_empty {
449-
let _ = path.pop();
450-
} else {
451-
path.push('"')
452-
}
453-
if chars.next() != Some(']') {
454-
return Err(ErrorCode::InvalidArgument("Expected ']' after string"));
455-
}
456-
} else {
457-
while let Some(&c) = chars.peek() {
458-
if c == ']' {
459-
break;
460-
}
461-
path.write_char(c)?;
462-
chars.next();
463-
}
464-
if chars.next() != Some(']') {
465-
return Err(ErrorCode::InvalidArgument(
466-
"Expected ']' after index".to_string(),
467-
));
468-
}
469-
};
470-
471-
path.write_char(',')?;
472-
}
473-
if path.len() > 1 {
474-
let _ = path.pop();
475-
}
476-
path.write_char('}')?;
477-
478-
if chars.next().is_some() {
479-
return Err(ErrorCode::InvalidArgument("Unexpected trailing characters"));
480-
}
481-
Ok(path)
482-
}
483-
}
484-
485-
#[cfg(test)]
486-
mod test {
487-
use databend_common_exception::Result;
488-
489-
use crate::Binder;
490-
491-
#[test]
492-
fn test_convert_array_index() -> Result<()> {
493-
let success = vec![
494-
("['a']", "{\"a\"}"),
495-
("['a'][0]", "{\"a\",0}"),
496-
("['a']['b']", "{\"a\",\"b\"}"),
497-
("['a']['some_word']", "{\"a\",\"some_word\"}"),
498-
("['a'][42]['long_string']", "{\"a\",42,\"long_string\"}"),
499-
("['a'][0]['b']['c']", "{\"a\",0,\"b\",\"c\"}"),
500-
("['key']['value'][123]", "{\"key\",\"value\",123}"),
501-
("[0]", "{0}"),
502-
];
503-
let failure = vec!["invalid", "['a'", "['a']extra"];
504-
for (case, result) in success {
505-
assert_eq!(result, Binder::convert_array_index(case)?);
506-
}
507-
for case in failure {
508-
assert!(Binder::convert_array_index(case).is_err());
509-
}
510-
511-
Ok(())
512-
}
513328
}

‎src/query/sql/src/planner/binder/bind_table_reference/bind_table_function.rs

+6-2
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,9 @@ impl Binder {
152152
false,
153153
false,
154154
None,
155-
);
155+
&mut bind_context.virtual_column_context,
156+
&mut bind_context.columns,
157+
)?;
156158

157159
let (s_expr, mut bind_context) =
158160
self.bind_base_table(bind_context, "system", table_index, None, sample)?;
@@ -214,7 +216,9 @@ impl Binder {
214216
false,
215217
false,
216218
None,
217-
);
219+
&mut bind_context.virtual_column_context,
220+
&mut bind_context.columns,
221+
)?;
218222

219223
let (s_expr, mut bind_context) =
220224
self.bind_base_table(bind_context, "system", table_index, None, &None)?;

‎src/query/sql/src/planner/binder/copy_into_table.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ impl Binder {
239239
#[async_backtrace::framed]
240240
async fn bind_copy_into_table_from_location(
241241
&mut self,
242-
bind_ctx: &BindContext,
242+
bind_ctx: &mut BindContext,
243243
plan: CopyIntoTablePlan,
244244
) -> Result<Plan> {
245245
let use_query = matches!(&plan.stage_table_info.stage_info.file_format_params,
@@ -422,7 +422,7 @@ impl Binder {
422422
#[async_backtrace::framed]
423423
async fn bind_copy_from_query_into_table(
424424
&mut self,
425-
bind_context: &BindContext,
425+
bind_context: &mut BindContext,
426426
mut plan: CopyIntoTablePlan,
427427
select_list: &[SelectTarget],
428428
alias: &Option<TableAlias>,

0 commit comments

Comments
 (0)