Skip to content

Commit a04a6b8

Browse files
DanCodedThisDanCodedThis
authored
engine: Explain fix (#1891)
* explain fix * explain fix insta tests * explain fix remove redundant tracing * explain fix flaky test * explain fix flaky test + cargo clippy * cargo clippy toolchain fix * cargo fmt * cargo fmt * cargo fmt * insta tests snapshots * insta tests snapshots delete * insta tests snapshots remake * insta tests snapshots columns fix * explain fix insta test snapshots * explain fix whitespace insta test snapshots * tests fix * insta tests fix
1 parent 572a434 commit a04a6b8

42 files changed

Lines changed: 497 additions & 132 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.github/workflows/tests.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,6 @@ jobs:
4444
sudo apt install -y llvm-18 libclang-18-dev
4545
sudo ldconfig
4646
- name: cargo test
47+
env:
48+
COLUMNS: "80"
4749
run: RUSTFLAGS="-C linker=clang -C link-arg=-fuse-ld=lld" cargo test --profile=ci --workspace --all-features --all-targets

‎Cargo.lock‎

Lines changed: 54 additions & 34 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/api-iceberg-rest/src/schemas.rs‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,7 @@ use serde::{Deserialize, Serialize};
1616
pub fn to_schema(request: CreateNamespaceRequest, db: String) -> MetastoreSchema {
1717
MetastoreSchema {
1818
ident: MetastoreSchemaIdent {
19-
schema: request
20-
.namespace
21-
.first()
22-
.unwrap_or(&String::new())
23-
.to_string(),
19+
schema: request.namespace.first().unwrap_or(&String::new()).clone(),
2420
database: db,
2521
},
2622
properties: request.properties,

‎crates/api-ui/src/lib.rs‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ impl Display for SearchParameters {
5050
.offset
5151
.map_or_else(|| str.to_string(), |offset| format!("{str}offset={offset}"));
5252
let str = self.limit.map_or_else(
53-
|| str.to_string(),
53+
|| str.clone(),
5454
|limit| {
5555
format!(
5656
"{str}{}limit={limit}",
@@ -59,7 +59,7 @@ impl Display for SearchParameters {
5959
},
6060
);
6161
let str = self.search.clone().map_or_else(
62-
|| str.to_string(),
62+
|| str.clone(),
6363
|search| {
6464
format!(
6565
"{str}{}search={search}",
@@ -68,7 +68,7 @@ impl Display for SearchParameters {
6868
},
6969
);
7070
let str = self.order_by.clone().map_or_else(
71-
|| str.to_string(),
71+
|| str.clone(),
7272
|order_by| {
7373
format!(
7474
"{str}{}orderBy={order_by}",
@@ -77,7 +77,7 @@ impl Display for SearchParameters {
7777
},
7878
);
7979
let str = self.order_direction.map_or_else(
80-
|| str.to_string(),
80+
|| str.clone(),
8181
|order_direction| {
8282
format!(
8383
"{str}{}orderDirection={order_direction}",
@@ -91,17 +91,13 @@ impl Display for SearchParameters {
9191

9292
#[derive(Debug, Deserialize, ToSchema, Copy, Clone)]
9393
#[serde(rename_all = "UPPERCASE")]
94+
#[derive(Default)]
9495
pub enum OrderDirection {
9596
ASC,
97+
#[default]
9698
DESC,
9799
}
98100

99-
impl Default for OrderDirection {
100-
fn default() -> Self {
101-
Self::DESC
102-
}
103-
}
104-
105101
impl Display for OrderDirection {
106102
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
107103
match self {

‎crates/api-ui/src/tables/handlers.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ pub async fn get_table_preview_data(
263263
.collect();
264264

265265
preview_data_columns.push(TablePreviewDataColumn {
266-
name: schema.field(i).name().to_string(),
266+
name: schema.field(i).name().clone(),
267267
rows: preview_data_rows,
268268
});
269269
}

‎crates/api-ui/src/tests/auth.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ async fn test_account_ok() {
454454
),
455455
]),
456456
&format!("http://{addr}/ui/auth/account"),
457-
String::new().to_string(),
457+
String::new().clone(),
458458
)
459459
.await
460460
.expect("Failed to get account");
@@ -484,7 +484,7 @@ async fn test_account_unauthorized() {
484484
HeaderValue::from_static("application/json"),
485485
)]),
486486
&format!("http://{addr}/ui/auth/account"),
487-
String::new().to_string(),
487+
String::new().clone(),
488488
)
489489
.await
490490
.expect_err("Account should fail");

‎crates/core-executor/src/query.rs‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,11 @@ impl UserQuery {
287287

288288
#[instrument(name = "UserQuery::postprocess_query_statement", level = "trace", err)]
289289
pub fn postprocess_query_statement_with_validation(statement: &mut DFStatement) -> Result<()> {
290+
let statement = if let DFStatement::Explain(explain) = statement {
291+
explain.statement.as_mut()
292+
} else {
293+
statement
294+
};
290295
if let DFStatement::Statement(value) = statement {
291296
rlike_regexp_expr_rewriter::visit(value);
292297
functions_rewriter::visit(value);
@@ -953,7 +958,7 @@ impl UserQuery {
953958
let catalog = self.get_catalog(&catalog_name).map_err(|_| {
954959
ex_error::CatalogNotFoundSnafu {
955960
operation_on: OperationOn::Table(OperationType::Create),
956-
catalog: catalog_name.to_string(),
961+
catalog: catalog_name.clone(),
957962
}
958963
.build()
959964
})?;
@@ -2546,17 +2551,17 @@ impl UserQuery {
25462551
.await
25472552
.context(ex_error::MetastoreSnafu)?
25482553
.context(ex_error::TableNotFoundSnafu {
2549-
schema: ident.schema.to_string(),
2550-
table: ident.table.to_string(),
2554+
schema: ident.schema.clone(),
2555+
table: ident.table.clone(),
25512556
})?;
25522557
let volume = self
25532558
.metastore
25542559
.volume_for_table(&ident)
25552560
.await
25562561
.context(ex_error::MetastoreSnafu)?
25572562
.context(ex_error::TableNotFoundSnafu {
2558-
schema: ident.schema.to_string(),
2559-
table: ident.table.to_string(),
2563+
schema: ident.schema.clone(),
2564+
table: ident.table.clone(),
25602565
})?;
25612566
let setup_query = match volume.volume.clone() {
25622567
VolumeType::S3(s3_volume) => {
@@ -3219,10 +3224,8 @@ pub fn merge_clause_projection<S: ContextProvider>(
32193224
if values.rows.len() != 1 {
32203225
return Err(ex_error::MergeInsertOnlyOneRowSnafu.build());
32213226
}
3222-
let mut all_columns: HashSet<String> = target_schema
3223-
.iter()
3224-
.map(|x| x.1.name().to_string())
3225-
.collect();
3227+
let mut all_columns: HashSet<String> =
3228+
target_schema.iter().map(|x| x.1.name().clone()).collect();
32263229
for (column, value) in insert.columns.iter().zip(
32273230
values
32283231
.rows
@@ -3406,7 +3409,8 @@ async fn target_filter_expression(
34063409
table: &DataFusionTable,
34073410
) -> Result<Option<datafusion_expr::Expr>> {
34083411
#[allow(clippy::unwrap_used)]
3409-
let table = match table.tabular.read().unwrap().clone() {
3412+
let value = table.tabular.read().unwrap().clone();
3413+
let table = match value {
34103414
Tabular::Table(table) => Ok(table),
34113415
_ => MergeSourceNotSupportedSnafu.fail(),
34123416
}?;

‎crates/core-executor/src/tests/query.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,11 @@ macro_rules! test_query {
199199
settings.set_omit_expression(true);
200200
settings.set_prepend_module_to_snapshot(false);
201201
settings.set_snapshot_path(concat!("snapshots", "/") $(.to_owned() + $user_snapshot_path)?);
202+
settings.add_filter(r"/[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}\.parquet", "/[UUID].parquet");
203+
settings.add_filter(r"/testing/data/[0-9a-fA-F]{4,8}/", "/testing/data/[HEX]/");
204+
settings.add_filter(r"(?i)\b(metadata_load_time|time_elapsed_opening|time_elapsed_processing|time_elapsed_scanning_total|time_elapsed_scanning_until_data|elapsed_compute|bloom_filter_eval_time|page_index_eval_time|row_pushdown_eval_time|statistics_eval_time)\s*=\s*[0-9]+(?:\.[0-9]+)?\s*(?:ns|µs|us|ms|s)", "$1=[TIME]");
205+
settings.add_filter(r"(-{140})(-{1,})", "$1");
206+
settings.add_filter(r"( {110})( {1,})", "$1");
202207

203208
let setup: Vec<&str> = vec![$($($setup_queries),*)?];
204209
if !setup.is_empty() {

‎crates/core-executor/src/tests/snapshots/session/query_explain_select_limit.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ info: "Setup queries: SET datafusion.explain.logical_plan_only = true"
66
Ok(
77
[
88
"+--------------+---------------------------------------------------------------------------------------------------------------------+",
9-
"| plan_type | plan |",
9+
"| plan_type | plan |",
1010
"+--------------+---------------------------------------------------------------------------------------------------------------------+",
1111
"| logical_plan | Limit: skip=0, fetch=1 |",
1212
"| | TableScan: embucket.public.employee_table projection=[employee_id, last_name, first_name, department_id], fetch=1 |",

0 commit comments

Comments
 (0)