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.

Sign up for GitHub

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

Remove dead code in UnwrapCastExprRewriter that may mask errors #3703

Merged
merged 3 commits into from
Oct 7, 2022
Merged

Remove dead code in UnwrapCastExprRewriter that may mask errors #3703

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8385a3c
Remove dead check
alamb Oct 4, 2022
c863316
Merge remote-tracking branch 'apache/master' into alamb/avoid_unecess…
alamb Oct 6, 2022
139f08d
Further simplify
alamb Oct 6, 2022
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
4 changes: 0 additions & 4 deletions datafusion/optimizer/src/unwrap_cast_in_comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,6 @@ impl ExprRewriter for UnwrapCastExprRewriter {
let right = right.as_ref().clone();
let left_type = left.get_type(&self.schema);
let right_type = right.get_type(&self.schema);
// can't get the data type, just return the expr
if left_type.is_err() || right_type.is_err() {
return Ok(expr.clone());
}
Comment on lines -160 to -163
Copy link
Member

Choose a reason for hiding this comment

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

Do we know for sure that this is dead code versus untested code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know for sure. It is clearly untested

Copy link
Contributor

@liukun4515 liukun4515 Oct 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

agree with your comments

// 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?;
Copy link
Member

@andygrove andygrove Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)?;

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to throw the error when can't get_type of the expr, I think this fix maybe reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @alamb @andygrove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

think the change could be slightly larger, replacing lines 123 through 131 just with:

Done in 139f08d

Expand Down