Skip to content

Commit

Permalink
refactor(error): simplify not implemented error Instantiation
Browse files Browse the repository at this point in the history 
Signed-off-by: Bugen Zhao <[email protected]>
  • Loading branch information
@BugenZhao
BugenZhao committed Nov 23, 2023
1 parent 971f77c commit d37b87b
Show file tree
Hide file tree
Showing 35 changed files with 263 additions and 439 deletions.
78 changes: 67 additions & 11 deletions src/common/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,61 @@ impl Display for TrackingIssue {
}
}

#[derive(Error, Debug)]
#[error("Feature is not yet implemented: {feature}. {tracking_issue}")]
pub struct NotImplemented {
pub feature: String,
pub tracking_issue: TrackingIssue,
}

impl<S> From<S> for NotImplemented
where
S: Into<String>,
{
fn from(feature: S) -> Self {
Self::new(feature)
}
}

impl NotImplemented {
pub fn new(feature: impl Into<String>) -> Self {
Self::with_tracking_issue(feature, TrackingIssue::none())
}

pub fn with_tracking_issue(
feature: impl Into<String>,
tracking_issue: impl Into<TrackingIssue>,
) -> Self {
Self {
feature: feature.into(),
tracking_issue: tracking_issue.into(),
}
}
}

#[macro_export]
macro_rules! not_implemented {
(issue = $issue:expr, $($arg:tt)*) => {
$crate::error::NotImplemented::with_tracking_issue(
::std::format!($($arg)*),
$issue,
)
};
($($arg:tt)*) => {
not_implemented!(issue = None, $($arg)*)
};
}

#[macro_export(local_inner_macros)]
macro_rules! bail_not_implemented {
(issue = $issue:expr, $($arg:tt)*) => {
return Err(not_implemented!(issue = $issue, $($arg)*).into())
};
($($arg:tt)*) => {
bail_not_implemented!(issue = None, $($arg)*)
};
}

#[derive(Error, Debug)]
pub enum ErrorCode {
#[error("internal error: {0}")]
Expand All @@ -86,8 +141,8 @@ pub enum ErrorCode {
#[backtrace]
BoxedError,
),
#[error("Feature is not yet implemented: {0}\n{1}")]
NotImplemented(String, TrackingIssue),
#[error(transparent)]
NotImplemented(#[from] NotImplemented),
// Tips: Use this only if it's intended to reject the query
#[error("Not supported: {0}\nHINT: {1}")]
NotSupported(String, String),
Expand Down Expand Up @@ -195,6 +250,13 @@ pub enum ErrorCode {
UnrecognizedConfigurationParameter(String),
}

// TODO(error-handling): automatically generate this impl.
impl From<NotImplemented> for RwError {
fn from(value: NotImplemented) -> Self {
ErrorCode::from(value).into()
}
}

pub fn internal_error(msg: impl Into<String>) -> RwError {
ErrorCode::InternalError(msg.into()).into()
}
Expand Down Expand Up @@ -468,14 +530,8 @@ macro_rules! ensure_eq {

#[macro_export]
macro_rules! bail {
($msg:literal $(,)?) => {
return Err($crate::error::anyhow_error!($msg).into())
};
($err:expr $(,)?) => {
return Err($crate::error::anyhow_error!($err).into())
};
($fmt:expr, $($arg:tt)*) => {
return Err($crate::error::anyhow_error!($fmt, $($arg)*).into())
($($arg:tt)*) => {
return Err($crate::error::anyhow_error!($($arg)*).into())
};
}

Expand Down Expand Up @@ -605,7 +661,7 @@ mod tests {
check_grpc_error(ErrorCode::TaskNotFound, Code::Internal);
check_grpc_error(ErrorCode::InternalError(String::new()), Code::Internal);
check_grpc_error(
ErrorCode::NotImplemented(String::new(), None.into()),
ErrorCode::NotImplemented(NotImplemented::new("test")),
Code::Internal,
);
}
Expand Down
19 changes: 7 additions & 12 deletions src/common/src/types/to_binary.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,16 @@ use bytes::{Bytes, BytesMut};
use postgres_types::{ToSql, Type};

use super::{DataType, DatumRef, ScalarRefImpl, F32, F64};
use crate::error::TrackingIssue;
use crate::error::NotImplemented;

/// Error type for [`ToBinary`] trait.
#[derive(thiserror::Error, Debug)]
pub enum ToBinaryError {
#[error(transparent)]
ToSql(Box<dyn std::error::Error + Send + Sync>),

#[error("Feature is not yet implemented: {0}\n{1}")]
NotImplemented(String, TrackingIssue),
#[error(transparent)]
NotImplemented(#[from] NotImplemented),
}

pub type Result<T> = std::result::Result<T, ToBinaryError>;
Expand Down Expand Up @@ -87,15 +87,10 @@ impl ToBinary for ScalarRefImpl<'_> {
ScalarRefImpl::Time(v) => v.to_binary_with_type(ty),
ScalarRefImpl::Bytea(v) => v.to_binary_with_type(ty),
ScalarRefImpl::Jsonb(v) => v.to_binary_with_type(ty),
ScalarRefImpl::Struct(_) | ScalarRefImpl::List(_) => {
Err(ToBinaryError::NotImplemented(
format!(
"the pgwire extended-mode encoding for {} is unsupported",
ty
),
Some(7949).into(),
))
}
ScalarRefImpl::Struct(_) | ScalarRefImpl::List(_) => bail_not_implemented!(
issue = 7949,
"the pgwire extended-mode encoding for {ty} is unsupported"
),
}
}
}
Expand Down
7 changes: 2 additions & 5 deletions src/frontend/src/binder/expr/binary_op.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use risingwave_common::bail_not_implemented;
use risingwave_common::error::{ErrorCode, Result};
use risingwave_common::types::DataType;
use risingwave_sqlparser::ast::{BinaryOperator, Expr};
Expand Down Expand Up @@ -186,11 +187,7 @@ impl Binder {
func_types.push(ExprType::Not);
ExprType::RegexpEq
}
_ => {
return Err(
ErrorCode::NotImplemented(format!("binary op: {:?}", op), 112.into()).into(),
)
}
_ => bail_not_implemented!(issue = 112, "binary op: {:?}", op),
};
func_types.push(final_type);
Ok(func_types)
Expand Down
104 changes: 34 additions & 70 deletions src/frontend/src/binder/expr/function.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use risingwave_common::catalog::{INFORMATION_SCHEMA_SCHEMA_NAME, PG_CATALOG_SCHE
use risingwave_common::error::{ErrorCode, Result, RwError};
use risingwave_common::session_config::USER_NAME_WILD_CARD;
use risingwave_common::types::{DataType, ScalarImpl, Timestamptz};
use risingwave_common::{GIT_SHA, RW_VERSION};
use risingwave_common::{bail_not_implemented, not_implemented, GIT_SHA, RW_VERSION};
use risingwave_expr::aggregate::{agg_kinds, AggKind};
use risingwave_expr::window_function::{
Frame, FrameBound, FrameBounds, FrameExclusion, WindowFuncKind,
Expand Down Expand Up @@ -68,28 +68,22 @@ impl Binder {
// FIXME: handle schema correctly, so that the functions are hidden if the schema is not in the search path.
let function_name = name.real_value();
if function_name != "_pg_expandarray" {
return Err(ErrorCode::NotImplemented(
format!("Unsupported function name under schema: {}", schema_name),
12422.into(),
)
.into());
bail_not_implemented!(
issue = 12422,
"Unsupported function name under schema: {}",
schema_name
);
}
function_name
} else {
return Err(ErrorCode::NotImplemented(
format!("Unsupported function name under schema: {}", schema_name),
12422.into(),
)
.into());
bail_not_implemented!(
issue = 12422,
"Unsupported function name under schema: {}",
schema_name
);
}
}
_ => {
return Err(ErrorCode::NotImplemented(
format!("qualified function: {}", f.name),
112.into(),
)
.into());
}
_ => bail_not_implemented!(issue = 112, "qualified function {}", f.name),
};

// agg calls
Expand Down Expand Up @@ -141,11 +135,11 @@ impl Binder {
))
.into());
} else if f.over.is_some() {
return Err(ErrorCode::NotImplemented(
format!("Unrecognized window function: {}", function_name),
8961.into(),
)
.into());
bail_not_implemented!(
issue = 8961,
"Unrecognized window function: {}",
function_name
);
}

// table function
Expand Down Expand Up @@ -272,25 +266,13 @@ impl Binder {
.and_then(|expr| expr.enforce_bool_clause("FILTER"))?;
self.context.clause = clause;
if expr.has_subquery() {
return Err(ErrorCode::NotImplemented(
"subquery in filter clause".to_string(),
None.into(),
)
.into());
bail_not_implemented!("subquery in filter clause");
}
if expr.has_agg_call() {
return Err(ErrorCode::NotImplemented(
"aggregation function in filter clause".to_string(),
None.into(),
)
.into());
bail_not_implemented!("aggregation function in filter clause");
}
if expr.has_table_function() {
return Err(ErrorCode::NotImplemented(
"table function in filter clause".to_string(),
None.into(),
)
.into());
bail_not_implemented!("table function in filter clause");
}
Condition::with_expr(expr)
}
Expand Down Expand Up @@ -348,11 +330,7 @@ impl Binder {
.flatten_ok()
.try_collect()?;
if args.iter().any(|arg| arg.as_literal().is_none()) {
return Err(ErrorCode::NotImplemented(
"non-constant direct arguments for ordered-set aggregation is not supported now".to_string(),
None.into()
)
.into());
bail_not_implemented!("non-constant direct arguments for ordered-set aggregation is not supported now");
}
args
};
Expand Down Expand Up @@ -470,12 +448,7 @@ impl Binder {
// restrict arguments[1..] to be constant because we don't support multiple distinct key
// indices for now
if args.iter().skip(1).any(|arg| arg.as_literal().is_none()) {
return Err(ErrorCode::NotImplemented(
"non-constant arguments other than the first one for DISTINCT aggregation is not supported now"
.to_string(),
None.into(),
)
.into());
bail_not_implemented!("non-constant arguments other than the first one for DISTINCT aggregation is not supported now");
}

// restrict ORDER BY to align with PG, which says:
Expand Down Expand Up @@ -520,14 +493,11 @@ impl Binder {
match exclusion {
WindowFrameExclusion::CurrentRow => FrameExclusion::CurrentRow,
WindowFrameExclusion::Group | WindowFrameExclusion::Ties => {
return Err(ErrorCode::NotImplemented(
format!(
"window frame exclusion `{}` is not supported yet",
exclusion
),
9124.into(),
)
.into());
bail_not_implemented!(
issue = 9124,
"window frame exclusion `{}` is not supported yet",
exclusion
);
}
WindowFrameExclusion::NoOthers => FrameExclusion::NoOthers,
}
Expand Down Expand Up @@ -556,14 +526,11 @@ impl Binder {
FrameBounds::Rows(start, end)
}
WindowFrameUnits::Range | WindowFrameUnits::Groups => {
return Err(ErrorCode::NotImplemented(
format!(
"window frame in `{}` mode is not supported yet",
frame.units
),
9124.into(),
)
.into());
bail_not_implemented!(
issue = 9124,
"window frame in `{}` mode is not supported yet",
frame.units
);
}
};
if !bounds.is_valid() {
Expand Down Expand Up @@ -972,10 +939,7 @@ impl Binder {
.map_err(|_| no_match_err)?;

let ExprImpl::Literal(literal) = &input else {
return Err(ErrorCode::NotImplemented(
"Only boolean literals are supported in `current_schemas`.".to_string(), None.into()
)
.into());
bail_not_implemented!("Only boolean literals are supported in `current_schemas`.");
};

let Some(bool) = literal.get_data().as_ref().map(|bool| bool.clone().into_bool()) else {
Expand Down Expand Up @@ -1277,7 +1241,7 @@ impl Binder {
)
};

ErrorCode::NotImplemented(err_msg, 112.into()).into()
not_implemented!(issue = 112, "{}", err_msg).into()
}),
}
}
Expand Down
Loading

0 comments on commit d37b87b

Please sign in to comment.