Skip to content

Commit

Permalink
fix: set query_kind earlier to ensure it takes effect. (#17302)
Browse files Browse the repository at this point in the history
maybe related to the performance regression reported in issue 17284.
  • Loading branch information
youngsofun authored Jan 17, 2025
1 parent a37a790 commit 31c243b
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 9 deletions.
13 changes: 8 additions & 5 deletions src/query/sql/src/planner/planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ impl Planner {
#[fastrace::trace]
pub async fn plan_stmt(&mut self, stmt: &Statement, attach_query: bool) -> Result<Plan> {
let start = Instant::now();
let query_kind = get_query_kind(stmt);
let settings = self.ctx.get_settings();
// Step 3: Bind AST with catalog, and generate a pure logical SExpr
let name_resolution_ctx = NameResolutionContext::try_from(settings.as_ref())?;
Expand All @@ -243,8 +244,7 @@ impl Planner {
info!("logical plan from cache, time used: {:?}", start.elapsed());
if attach_query {
// update for clickhouse handler
self.ctx
.attach_query_str(get_query_kind(stmt), stmt.to_mask_sql());
self.ctx.attach_query_str(query_kind, stmt.to_mask_sql());
}
return Ok(plan.plan);
}
Expand All @@ -260,11 +260,14 @@ impl Planner {
)
.with_subquery_executor(self.query_executor.clone());

// Indicate binder there is no need to collect column statistics for the binding table.
// must attach before bind, because ParquetRSTable::create used it.
if attach_query {
self.ctx.attach_query_str(query_kind, stmt.to_mask_sql());
}
let plan = binder.bind(stmt).await?;
// attach again to avoid the query kind is overwritten by the subquery
if attach_query {
self.ctx
.attach_query_str(get_query_kind(stmt), stmt.to_mask_sql());
self.ctx.attach_query_str(query_kind, stmt.to_mask_sql());
}

// Step 4: Optimize the SExpr with optimizers, and generate optimized physical SExpr
Expand Down
16 changes: 12 additions & 4 deletions src/query/storages/parquet/src/parquet_rs/parquet_table/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use databend_common_catalog::table::DummyColumnStatisticsProvider;
use databend_common_catalog::table::Table;
use databend_common_catalog::table::TableStatistics;
use databend_common_catalog::table_context::TableContext;
use databend_common_exception::ErrorCode;
use databend_common_exception::Result;
use databend_common_expression::TableField;
use databend_common_expression::TableSchema;
Expand Down Expand Up @@ -141,10 +142,17 @@ impl ParquetRSTable {

// If the query is `COPY`, we don't need to collect column statistics.
// It's because the only transform could be contained in `COPY` command is projection.
let need_stats_provider = !matches!(
query_kind,
QueryKind::CopyIntoTable | QueryKind::CopyIntoLocation
);
let need_stats_provider = match query_kind {
QueryKind::CopyIntoTable | QueryKind::CopyIntoLocation => true,
QueryKind::Unknown => {
// add this branch to ensure query_kind is set
return Err(ErrorCode::Internal(
"Unexpected QueryKind::Unknown: query_kind was not properly set before calling ParquetRSTable::create.",
));
}
_ => false,
};

let max_threads = settings.get_max_threads()? as usize;
let max_memory_usage = settings.get_max_memory_usage()?;

Expand Down

0 comments on commit 31c243b

Please sign in to comment.