alamb commented on code in PR #11958:
URL: https://github.com/apache/datafusion/pull/11958#discussion_r1714394743
##########
datafusion/common/Cargo.toml:
##########
@@ -60,6 +60,7 @@ libc = "0.2.140"
num_cpus = { workspace = true }
object_store = { workspace = true, optional = true }
parquet = { workspace = true, optional = true, default-features = true }
+paste = "1.0.15"
Review Comment:
for anyone else reviewing this PR, this is not a new dependency as it was
already used transitively by other packages
##########
datafusion/common/src/error.rs:
##########
@@ -501,37 +494,37 @@ macro_rules! with_dollar_sign {
/// `NAME_DF_ERR` - macro name for wrapping DataFusionError::*. Needed to
keep backtrace opportunity
/// in construction where DataFusionError::* used directly, like `map_err`,
`ok_or_else`, etc
macro_rules! make_error {
- ($NAME_ERR:ident, $NAME_DF_ERR: ident, $ERR:ident) => {
- with_dollar_sign! {
- ($d:tt) => {
- /// Macro wraps `$ERR` to add backtrace feature
- #[macro_export]
- macro_rules! $NAME_DF_ERR {
- ($d($d args:expr),*) => {
- $crate::DataFusionError::$ERR(
- format!(
- "{}{}",
- format!($d($d args),*),
- $crate::DataFusionError::get_back_trace(),
- ).into()
- )
- }
+ ($NAME_ERR:ident, $NAME_DF_ERR: ident, $ERR:ident) => { make_error!(@inner
($), $NAME_ERR, $NAME_DF_ERR, $ERR); };
+ (@inner ($d:tt), $NAME_ERR:ident, $NAME_DF_ERR:ident, $ERR:ident) => {
+ ::paste::paste!{
+ /// Macro wraps `$ERR` to add backtrace feature
+ #[macro_export]
+ macro_rules! $NAME_DF_ERR {
+ ($d($d args:expr),*) => {
+ $crate::DataFusionError::$ERR(
+ ::std::format!(
+ "{}{}",
+ ::std::format!($d($d args),*),
+ $crate::DataFusionError::get_back_trace(),
+ ).into()
+ )
}
+ }
- /// Macro wraps Err(`$ERR`) to add backtrace feature
- #[macro_export]
- macro_rules! $NAME_ERR {
- ($d($d args:expr),*) => {
- Err($crate::DataFusionError::$ERR(
- format!(
- "{}{}",
- format!($d($d args),*),
- $crate::DataFusionError::get_back_trace(),
- ).into()
- ))
- }
+ /// Macro wraps Err(`$ERR`) to add backtrace feature
+ #[macro_export]
+ macro_rules! $NAME_ERR {
+ ($d($d args:expr),*) => {
+ Err($crate::[<_ $NAME_DF_ERR>]!($d($d args),*))
}
}
+
+ #[doc(hidden)]
+ #[allow(unused)]
Review Comment:
Why does this macro need to be marked as `#[allow(unused)]`? I thought these
macros are used in various places of the codebase
##########
datafusion/sql/src/unparser/plan.rs:
##########
@@ -359,18 +359,14 @@ impl Unparser<'_> {
.iter()
.map(|e| self.select_item_to_sql(e))
.collect::<Result<Vec<_>>>()?;
- match &on.sort_expr {
- Some(sort_expr) => {
- if let Some(query_ref) = query {
- query_ref
-
.order_by(self.sort_to_sql(sort_expr.clone())?);
- } else {
- return internal_err!(
- "Sort operator only valid in a statement
context."
- );
- }
+ if let Some(sort_expr) = &on.sort_expr {
Review Comment:
Another way I have seen this written is
```suggestion
if let Some(sort_expr) = on.sort_expr.as_ref() {
```
##########
datafusion/common/src/lib.rs:
##########
@@ -73,6 +73,18 @@ pub use table_reference::{ResolvedTableReference,
TableReference};
pub use unnest::UnnestOptions;
pub use utils::project_schema;
+// These are hidden from docs purely to avoid polluting the public view of
what this crate exports.
Review Comment:
👍
##########
datafusion/functions/src/datetime/common.rs:
##########
@@ -227,46 +229,34 @@ where
// if the first argument is a scalar utf8 all arguments are expected
to be scalar utf8
ColumnarValue::Scalar(scalar) => match scalar {
ScalarValue::Utf8(a) | ScalarValue::LargeUtf8(a) => {
- let mut val: Option<Result<ColumnarValue>> = None;
- let mut err: Option<DataFusionError> = None;
+ let a = a.as_ref();
+ // ASK: Why do we trust `a` to be non-null at this point?
+ let a = unwrap_or_internal_err!(a);
- match a {
- Some(a) => {
- // enumerate all the values finding the first one that
returns an Ok result
- for (pos, v) in args.iter().enumerate().skip(1) {
- if let ColumnarValue::Scalar(s) = v {
- if let ScalarValue::Utf8(x) |
ScalarValue::LargeUtf8(x) =
- s
- {
- if let Some(s) = x {
- match op(a.as_str(), s.as_str()) {
- Ok(r) => {
- val =
Some(Ok(ColumnarValue::Scalar(
- S::scalar(Some(op2(r))),
- )));
- break;
- }
- Err(e) => {
- err = Some(e);
- }
- }
- }
- } else {
- return exec_err!("Unsupported data type
{s:?} for function {name}, arg # {pos}");
- }
- } else {
- return exec_err!("Unsupported data type {v:?}
for function {name}, arg # {pos}");
+ let mut ret = None;
+
+ for (pos, v) in args.iter().enumerate().skip(1) {
+ let ColumnarValue::Scalar(
+ ScalarValue::Utf8(x) | ScalarValue::LargeUtf8(x),
+ ) = v
+ else {
+ return exec_err!("Unsupported data type {v:?} for
function {name}, arg # {pos}");
+ };
+
+ if let Some(s) = x {
+ match op(a.as_str(), s.as_str()) {
+ Ok(r) => {
+ ret =
Some(Ok(ColumnarValue::Scalar(S::scalar(Some(
+ op2(r),
+ )))));
+ break;
}
+ Err(e) => ret = Some(Err(e)),
}
}
- None => (),
}
- if let Some(v) = val {
- v
- } else {
- Err(err.unwrap())
Review Comment:
that is pretty strange -- I like your formulation `unwrap_or_internal_err`
much better
##########
datafusion/sql/src/parser.rs:
##########
@@ -523,9 +523,7 @@ impl<'a> DFParser<'a> {
Ok(n) => Ok(Value::Number(n, l)),
// The tokenizer should have ensured `n` is an integer
// so this should not be possible
- Err(e) => parser_err!(format!(
- "Unexpected error: could not parse '{n}' as number: {e}"
- )),
+ Err(e) => match e {},
Review Comment:
I don't understand this change -- doesn't `match e{}` just ignore `e` where
as before this would have returned an error?
##########
datafusion/common/src/lib.rs:
##########
@@ -73,6 +73,18 @@ pub use table_reference::{ResolvedTableReference,
TableReference};
pub use unnest::UnnestOptions;
pub use utils::project_schema;
+// These are hidden from docs purely to avoid polluting the public view of
what this crate exports.
Review Comment:
I double checked the docs locally:
##########
datafusion/functions/src/datetime/common.rs:
##########
@@ -227,46 +229,34 @@ where
// if the first argument is a scalar utf8 all arguments are expected
to be scalar utf8
ColumnarValue::Scalar(scalar) => match scalar {
ScalarValue::Utf8(a) | ScalarValue::LargeUtf8(a) => {
- let mut val: Option<Result<ColumnarValue>> = None;
- let mut err: Option<DataFusionError> = None;
+ let a = a.as_ref();
+ // ASK: Why do we trust `a` to be non-null at this point?
Review Comment:
I don't think we do trust a to be non-null -- the current code on main has
an explicitly `match` that handles `None` doesn't it? Maybe I am misreading the
diff
```rust
None => (),
```
🤔
However, clearly we don't have test coverage for this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
