Remove dead code in UnwrapCastExprRewriter that may mask errors
#3703
Conversation
| // can't get the data type, just return the expr | ||
| if left_type.is_err() || right_type.is_err() { | ||
| return Ok(expr.clone()); | ||
| } |
There was a problem hiding this comment.
Do we know for sure that this is dead code versus untested code?
There was a problem hiding this comment.
I do not know for sure. It is clearly untested
There was a problem hiding this comment.
Why we need to remove this code? @alamb
From the interface get_type which may return error like that:
Expr::Wildcard => Err(DataFusionError::Internal(
"Wildcard expressions are not valid in a logical query plan".to_owned(),
)),
Expr::QualifiedWildcard { .. } => Err(DataFusionError::Internal(
"QualifiedWildcard expressions are not valid in a logical query plan"
.to_owned(),
)),
Do we need to throw the error for this rule, when get an error with get_type of the expr?
There was a problem hiding this comment.
Why we need to remove this code? @alamb
My thinking was that any such error is the result of some other issue with the planning or other pass; Thus, if we ignore the error in the UnwrapCastExprRewriter pass we make it harder to find and debug the real underlying problem.
It also has no test coverage (no test fails when this code is removed) which also makes me think all it will do is make debugging future errors harder
@liukun4515 if you think strongly this code should still be here it is fine with me and I will close the PR
There was a problem hiding this comment.
Why we need to remove this code? @alamb
My thinking was that any such error is the result of some other issue with the planning or other pass; Thus, if we ignore the error in the
UnwrapCastExprRewriterpass we make it harder to find and debug the real underlying problem.It also has no test coverage (no test fails when this code is removed) which also makes me think all it will do is make debugging future errors harder
@liukun4515 if you think strongly this code should still be here it is fine with me and I will close the PR
agree with your comments
| // can't get the data type, just return the expr | ||
| if left_type.is_err() || right_type.is_err() { | ||
| return Ok(expr.clone()); | ||
| } | ||
| // Because the plan has been done the type coercion, the left and right must be equal | ||
| let left_type = left_type?; | ||
| let right_type = right_type?; |
There was a problem hiding this comment.
I reviewed this again, and I think this is safe to remove. If we have expressions where we cannot determine the types then it indicates that an earlier rule has introduced an invalid expression.
I think the change could be slightly larger, replacing lines 123 through 131 just with:
let left_type = left.get_type(&self.schema)?;
let right_type = right.get_type(&self.schema)?;
There was a problem hiding this comment.
If we need to throw the error when can't get_type of the expr, I think this fix maybe reasonable.
There was a problem hiding this comment.
think the change could be slightly larger, replacing lines 123 through 131 just with:
Done in 139f08d
alamb
changed the title
UnwrapCastExprRewriterUnwrapCastExprRewriter that may mask errors
|
Benchmark runs are scheduled for baseline = db8dd19 and contender = 4dd4d43. 4dd4d43 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
re #3690
Rationale for this change
While reviewing code I found this code which may be masking errors
What changes are included in this PR?
Do not silently ignore errors getting schema
Are there any user-facing changes?
No