Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(query): remove extra error code construction #13926

Merged
merged 5 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
disallowed-methods = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to add some comments for this clippy.toml, we can do it in another PR.

{ path = "std::panic::catch_unwind", reason = "Please use `common_base::runtime::catch_unwind` instead." },
{ path = "futures::FutureExt::catch_unwind", reason = "Please use `common_base::runtime::CatchUnwindFuture` instead." },
{ path = "num_traits::sign::Signed::is_positive", reason = "This returns true for 0.0 but false for 0." },
{ path = "num_traits::sign::Signed::is_negative", reason = "This returns true for -0.0 but false for 0." },
{ path = "num_traits::sign::Signed::signum", reason = "This returns 1.0 for 0.0 but 0 for 0." }
]

## TODO: enable it in next pr
# disallowed-macros = [
# { path = "lazy_static::lazy_static", reason = "Please use `std::sync::LazyLock` instead." },
# ]

avoid-breaking-exported-api = true
too-many-arguments-threshold = 10
upper-case-acronyms-aggressive = false
6 changes: 3 additions & 3 deletions src/common/arrow/src/arrow/array/dictionary/typed_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ impl<O: Offset> DictValue for Utf8Array<O> {
array
.as_any()
.downcast_ref::<Self>()
.ok_or(Error::InvalidArgumentError(
"could not convert array to dictionary value".into(),
))
.ok_or_else(|| {
Error::InvalidArgumentError("could not convert array to dictionary value".into())
})
.map(|arr| {
assert_eq!(
arr.null_count(),
Expand Down
2 changes: 1 addition & 1 deletion src/common/arrow/src/native/compression/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl TryFrom<&Compression> for CommonCompression {
}

impl CommonCompression {
pub fn to_compression(&self) -> Compression {
pub fn to_compression(self) -> Compression {
match self {
Self::None => Compression::None,
Self::Lz4 => Compression::Lz4,
Expand Down
4 changes: 3 additions & 1 deletion src/common/base/src/base/take_mut.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use std::panic;

use common_exception::Result;

use crate::runtime::catch_unwind;

/// copy from https://docs.rs/take_mut/0.2.2/take_mut/fn.take.html with some modifications.
/// if a panic occurs, the entire process will be aborted, as there's no valid `T` to put back into the `&mut T`.
pub fn take_mut<T, F>(mut_ref: &mut T, closure: F) -> Result<()>
Expand All @@ -24,7 +26,7 @@ where F: FnOnce(T) -> Result<T> {

unsafe {
let old_t = ptr::read(mut_ref);
let closure_result = panic::catch_unwind(panic::AssertUnwindSafe(|| closure(old_t)));
let closure_result = catch_unwind(panic::AssertUnwindSafe(|| closure(old_t)));

match closure_result {
Ok(Ok(new_t)) => {
Expand Down
1 change: 1 addition & 0 deletions src/common/base/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#![feature(backtrace_frames)]
#![feature(alloc_error_hook)]
#![feature(slice_swap_unchecked)]
#![feature(lint_reasons)]

pub mod base;
pub mod containers;
Expand Down
1 change: 1 addition & 0 deletions src/common/base/src/runtime/catch_unwind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use futures::future::BoxFuture;
use futures::FutureExt;

pub fn catch_unwind<F: FnOnce() -> R, R>(f: F) -> Result<R> {
#[expect(clippy::disallowed_methods)]
match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) {
Ok(res) => Ok(res),
Err(cause) => match cause.downcast_ref::<&'static str>() {
Expand Down
8 changes: 6 additions & 2 deletions src/common/storage/src/stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,16 @@ impl StageFilesInfo {
#[async_backtrace::framed]
pub async fn first_file(&self, operator: &Operator) -> Result<StageFileInfo> {
let mut files = self.list(operator, true, None).await?;
files.pop().ok_or(ErrorCode::BadArguments("no file found"))
files
.pop()
.ok_or_else(|| ErrorCode::BadArguments("no file found"))
}

pub fn blocking_first_file(&self, operator: &Operator) -> Result<StageFileInfo> {
let mut files = self.blocking_list(operator, true, None)?;
files.pop().ok_or(ErrorCode::BadArguments("no file found"))
files
.pop()
.ok_or_else(|| ErrorCode::BadArguments("no file found"))
}

pub fn blocking_list(
Expand Down
37 changes: 21 additions & 16 deletions src/meta/api/src/schema_api_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,18 +552,19 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
UndropDbHasNoHistory::new(&name_key.db_name),
)));
} else {
db_id_list_opt.ok_or(KVAppError::AppError(AppError::UndropDbHasNoHistory(
UndropDbHasNoHistory::new(&name_key.db_name),
)))?
db_id_list_opt.ok_or_else(|| {
KVAppError::AppError(AppError::UndropDbHasNoHistory(UndropDbHasNoHistory::new(
&name_key.db_name,
)))
})?
};

// Return error if there is no db id history.
let db_id =
*db_id_list
.last()
.ok_or(KVAppError::AppError(AppError::UndropDbHasNoHistory(
UndropDbHasNoHistory::new(&name_key.db_name),
)))?;
let db_id = *db_id_list.last().ok_or_else(|| {
KVAppError::AppError(AppError::UndropDbHasNoHistory(UndropDbHasNoHistory::new(
&name_key.db_name,
)))
})?;

// get db_meta of the last db id
let dbid = DatabaseId { db_id };
Expand Down Expand Up @@ -1745,9 +1746,11 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
UndropTableHasNoHistory::new(&tenant_dbname_tbname.table_name),
)));
} else {
tb_id_list_opt.ok_or(KVAppError::AppError(AppError::UndropTableHasNoHistory(
UndropTableHasNoHistory::new(&tenant_dbname_tbname.table_name),
)))?
tb_id_list_opt.ok_or_else(|| {
KVAppError::AppError(AppError::UndropTableHasNoHistory(
UndropTableHasNoHistory::new(&tenant_dbname_tbname.table_name),
))
})?
};

// Return error if there is no table id history.
Expand Down Expand Up @@ -2339,10 +2342,12 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
let (_, table_name_opt): (_, Option<DBIdTableName>) =
get_pb_value(self, &table_id_to_name).await?;

let dbid_tbname =
table_name_opt.ok_or(KVAppError::AppError(AppError::UnknownTableId(
UnknownTableId::new(table_id, "drop_table_by_id failed to find db_id"),
)))?;
let dbid_tbname = table_name_opt.ok_or_else(|| {
KVAppError::AppError(AppError::UnknownTableId(UnknownTableId::new(
table_id,
"drop_table_by_id failed to find db_id",
)))
})?;

let db_id = dbid_tbname.db_id;
let tbname = dbid_tbname.table_name.clone();
Expand Down
9 changes: 4 additions & 5 deletions src/meta/app/src/principal/file_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,12 @@ impl FileFormatOptionsAst {
fn take_type(&mut self) -> Result<StageFileFormatType> {
let typ = match self.options.remove("type") {
Some(t) => t,
None => self
.options
.remove("format")
.ok_or(ErrorCode::IllegalFileFormat(format!(
None => self.options.remove("format").ok_or_else(|| {
ErrorCode::IllegalFileFormat(format!(
"Missing type in file format options: {:?}",
self.options
)))?,
))
})?,
};
StageFileFormatType::from_str(&typ).map_err(ErrorCode::IllegalFileFormat)
}
Expand Down
6 changes: 3 additions & 3 deletions src/meta/proto-conv/src/schema_from_to_protobuf_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl FromToProto for ex::ComputedExpr {
fn from_pb(p: pb::ComputedExpr) -> Result<Self, Incompatible> {
reader_check_msg(p.ver, p.min_reader_ver)?;

let computed_expr = p.computed_expr.ok_or(Incompatible {
let computed_expr = p.computed_expr.ok_or_else(|| Incompatible {
reason: "Invalid ComputedExpr: .computed_expr can not be None".to_string(),
})?;

Expand Down Expand Up @@ -335,7 +335,7 @@ impl FromToProto for ex::types::NumberDataType {
fn from_pb(p: pb::Number) -> Result<Self, Incompatible> {
reader_check_msg(p.ver, p.min_reader_ver)?;

let num = p.num.ok_or(Incompatible {
let num = p.num.ok_or_else(|| Incompatible {
reason: "Invalid Number: .num can not be None".to_string(),
})?;

Expand Down Expand Up @@ -386,7 +386,7 @@ impl FromToProto for ex::types::DecimalDataType {
fn from_pb(p: pb::Decimal) -> Result<Self, Incompatible> {
reader_check_msg(p.ver, p.min_reader_ver)?;

let num = p.decimal.ok_or(Incompatible {
let num = p.decimal.ok_or_else(|| Incompatible {
reason: "Invalid Decimal: .decimal can not be None".to_string(),
})?;

Expand Down
2 changes: 1 addition & 1 deletion src/meta/proto-conv/src/table_from_to_protobuf_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ impl FromToProto for mt::TableMeta {
fn from_pb(p: pb::TableMeta) -> Result<Self, Incompatible> {
reader_check_msg(p.ver, p.min_reader_ver)?;

let schema = p.schema.ok_or(Incompatible {
let schema = p.schema.ok_or_else(|| Incompatible {
reason: "TableMeta.schema can not be None".to_string(),
})?;

Expand Down
6 changes: 3 additions & 3 deletions src/meta/service/src/meta_service/raft_service_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ impl RaftServiceImpl {

let _g = snapshot_recv_inflight(&addr).counter_guard();

let chunk = snapshot_req.chunk.ok_or(GrpcHelper::invalid_arg(
"SnapshotChunkRequest.chunk is None",
))?;
let chunk = snapshot_req
.chunk
.ok_or_else(|| GrpcHelper::invalid_arg("SnapshotChunkRequest.chunk is None"))?;

let (vote, snapshot_meta): (Vote, SnapshotMeta) =
GrpcHelper::parse(&snapshot_req.rpc_meta)?;
Expand Down
2 changes: 1 addition & 1 deletion src/query/ast/src/ast/statements/copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ impl UriLocation {
hostname.to_string()
}
})
.ok_or(common_exception::ErrorCode::BadArguments("invalid uri"))?;
.ok_or_else(|| common_exception::ErrorCode::BadArguments("invalid uri"))?;

let path = if parsed.path().is_empty() {
"/".to_string()
Expand Down
6 changes: 3 additions & 3 deletions src/query/catalog/src/plan/internal_column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ impl BlockMetaInfo for InternalColumnMeta {

impl InternalColumnMeta {
pub fn from_meta(info: &BlockMetaInfoPtr) -> Result<&InternalColumnMeta> {
InternalColumnMeta::downcast_ref_from(info).ok_or(ErrorCode::Internal(
"Cannot downcast from BlockMetaInfo to InternalColumnMeta.",
))
InternalColumnMeta::downcast_ref_from(info).ok_or_else(|| {
ErrorCode::Internal("Cannot downcast from BlockMetaInfo to InternalColumnMeta.")
})
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/query/catalog/src/plan/stream_column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ impl BlockMetaInfo for StreamColumnMeta {

impl StreamColumnMeta {
pub fn from_meta(info: &BlockMetaInfoPtr) -> Result<&StreamColumnMeta> {
StreamColumnMeta::downcast_ref_from(info).ok_or(ErrorCode::Internal(
"Cannot downcast from BlockMetaInfo to StreamColumnMeta.",
))
StreamColumnMeta::downcast_ref_from(info).ok_or_else(|| {
ErrorCode::Internal("Cannot downcast from BlockMetaInfo to StreamColumnMeta.")
})
}

pub fn build_origin_block_id(&self) -> Value<AnyType> {
Expand Down
6 changes: 3 additions & 3 deletions src/query/ee/src/stream/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,13 @@ impl StreamHandler for RealStreamHandler {
let stream_opts = stream.get_table_info().options();
let stream_table_name = stream_opts
.get(OPT_KEY_TABLE_NAME)
.ok_or(ErrorCode::IllegalStream(format!("Illegal stream '{name}'")))?;
.ok_or_else(|| ErrorCode::IllegalStream(format!("Illegal stream '{name}'")))?;
let stream_database_name = stream_opts
.get(OPT_KEY_DATABASE_NAME)
.ok_or(ErrorCode::IllegalStream(format!("Illegal stream '{name}'")))?;
.ok_or_else(|| ErrorCode::IllegalStream(format!("Illegal stream '{name}'")))?;
let stream_table_id = stream_opts
.get(OPT_KEY_TABLE_ID)
.ok_or(ErrorCode::IllegalStream(format!("Illegal stream '{name}'")))?
.ok_or_else(|| ErrorCode::IllegalStream(format!("Illegal stream '{name}'")))?
.parse::<u64>()?;
if stream_table_name != &plan.table_name
|| stream_database_name != &plan.table_database
Expand Down
10 changes: 4 additions & 6 deletions src/query/expression/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,13 +446,11 @@ pub fn try_check_function<Index: ColumnIndex>(
.map(|max_generic_idx| {
(0..max_generic_idx + 1)
.map(|idx| {
subst
.0
.get(&idx)
.cloned()
.ok_or(ErrorCode::from_string_no_backtrace(format!(
subst.0.get(&idx).cloned().ok_or_else(|| {
ErrorCode::from_string_no_backtrace(format!(
"unable to resolve generic T{idx}"
)))
))
})
})
.collect::<Result<Vec<_>>>()
})
Expand Down
5 changes: 2 additions & 3 deletions src/query/expression/src/types/number.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use enum_as_inner::EnumAsInner;
use itertools::Itertools;
use lexical_core::ToLexicalWithOptions;
use num_traits::NumCast;
use num_traits::Signed;
use ordered_float::OrderedFloat;
use serde::Deserialize;
use serde::Serialize;
Expand Down Expand Up @@ -479,8 +478,8 @@ impl NumberScalar {
pub fn is_positive(&self) -> bool {
crate::with_integer_mapped_type!(|NUM_TYPE| match self {
NumberScalar::NUM_TYPE(num) => *num > 0,
NumberScalar::Float32(num) => num.is_positive(),
NumberScalar::Float64(num) => num.is_positive(),
NumberScalar::Float32(num) => num.is_sign_positive(),
NumberScalar::Float64(num) => num.is_sign_positive(),
})
}

Expand Down
20 changes: 10 additions & 10 deletions src/query/expression/src/utils/date_helper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,10 +275,8 @@ fn add_years_base(year: i32, month: u32, day: u32, delta: i64) -> Result<NaiveDa
if std::intrinsics::unlikely(month == 2 && day == 29) {
new_day = last_day_of_year_month(new_year, month);
}
NaiveDate::from_ymd_opt(new_year, month, new_day).ok_or(format!(
"Overflow on date YMD {}-{}-{}.",
new_year, month, new_day
))
NaiveDate::from_ymd_opt(new_year, month, new_day)
.ok_or_else(|| format!("Overflow on date YMD {}-{}-{}.", new_year, month, new_day))
}

fn add_months_base(year: i32, month: u32, day: u32, delta: i64) -> Result<NaiveDate, String> {
Expand All @@ -296,12 +294,14 @@ fn add_months_base(year: i32, month: u32, day: u32, delta: i64) -> Result<NaiveD
last_day_of_year_month(new_year, (new_month0 + 1) as u32),
);

NaiveDate::from_ymd_opt(new_year, (new_month0 + 1) as u32, new_day).ok_or(format!(
"Overflow on date YMD {}-{}-{}.",
new_year,
new_month0 + 1,
new_day
))
NaiveDate::from_ymd_opt(new_year, (new_month0 + 1) as u32, new_day).ok_or_else(|| {
format!(
"Overflow on date YMD {}-{}-{}.",
new_year,
new_month0 + 1,
new_day
)
})
}

// Get the last day of the year month, could be 28(non leap Feb), 29(leap year Feb), 30 or 31
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ where
.collect::<Vec<_>>();

if blocks.len() == 1 {
let block = blocks.get_mut(0).ok_or(ErrorCode::Internal("It's a bug"))?;
let block = blocks
.get_mut(0)
.ok_or_else(|| ErrorCode::Internal("It's a bug"))?;
if self.order_col_generated {
// Need to remove order column.
block.pop_columns(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ impl AsyncSink for ExchangeWriterSink {
None => Err(ErrorCode::Internal(
"ExchangeWriterSink only recv ExchangeSerializeMeta.",
)),
Some(block_meta) => ExchangeSerializeMeta::downcast_from(block_meta).ok_or(
ErrorCode::Internal("ExchangeWriterSink only recv ExchangeSerializeMeta."),
),
Some(block_meta) => ExchangeSerializeMeta::downcast_from(block_meta).ok_or_else(|| {
ErrorCode::Internal("ExchangeWriterSink only recv ExchangeSerializeMeta.")
}),
}?;

let mut bytes = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/query/service/src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl AuthMgr {
let ensure_user = jwt
.custom
.ensure_user
.ok_or(ErrorCode::AuthenticateFailure(e.message()))?;
.ok_or_else(|| ErrorCode::AuthenticateFailure(e.message()))?;
// create a new user if not exists
let mut user_info = UserInfo::new(&user_name, "%", AuthInfo::JWT);
if let Some(ref roles) = ensure_user.roles {
Expand Down
15 changes: 7 additions & 8 deletions src/query/service/src/interpreters/interpreter_delete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,13 @@ impl Interpreter for DeleteInterpreter {
(None, vec![])
};

let fuse_table =
tbl.as_any()
.downcast_ref::<FuseTable>()
.ok_or(ErrorCode::Unimplemented(format!(
"table {}, engine type {}, does not support DELETE FROM",
tbl.name(),
tbl.get_table_info().engine(),
)))?;
let fuse_table = tbl.as_any().downcast_ref::<FuseTable>().ok_or_else(|| {
ErrorCode::Unimplemented(format!(
"table {}, engine type {}, does not support DELETE FROM",
tbl.name(),
tbl.get_table_info().engine(),
))
})?;

let mut build_res = PipelineBuildResult::create();
let query_row_id_col = !self.plan.subquery_desc.is_empty();
Expand Down
Loading
Loading