chore: cleanup deprecated API since version <= 40
#15027
Conversation
github-actions
bot
added
logical-expr
qazxcdswe123
force-pushed
the
cleanup-deprecation
branch
from
26f87ea to
9653098
Compare
|
😍 thank you |
qazxcdswe123
force-pushed
the
cleanup-deprecation
branch
from
9653098 to
d0947f1
Compare
| @@ -329,19 +311,10 @@ fn optimize_plan_node( | |||
| config: &dyn OptimizerConfig, | |||
| ) -> Result<Transformed<LogicalPlan>> { | |||
| if rule.supports_rewrite() { | |||
| return rule.rewrite(plan, config); | |||
| rule.rewrite(plan, config) | |||
| } else { | |||
There was a problem hiding this comment.
potential semanic change here
There was a problem hiding this comment.
Can we please make this return an internal error rather than silently error?
There was a problem hiding this comment.
Actually thinking about this more I think we should simply always call rule.rewrite now (as it is required to implement now)
I am sorry about this @qazxcdswe123 but could you maybe revert the changes to OptimizerRule in this PR (and we can work on that in a follow on PR?)
There was a problem hiding this comment.
Done.
How should the Optimizer part behave? Should it just rule.rewrite, I think some of the comment is also out of date then
There was a problem hiding this comment.
lgtm thanks @qazxcdswe123
@alamb I'm feeling that having removed the deprecated code and not having migration instructions in Migration guide it would be quite a challenge to migrate from version 39 to 46 as we do not keep any traces/hints how API evolved and what is the new API
In my opinion any additional small burden related to this API removal (of APIs that have been deprecated for 6 months) is likely tiny compared to all the other major changes. Thus I don't think adding a migration guide adds much value here -- the fact the compiler has been telling people for months it is deprecated is the migration guide |
One exception is the optmizer part, which will be addressed in another commit
qazxcdswe123
force-pushed
the
cleanup-deprecation
branch
from
d0947f1 to
f37f8b0
Compare
|
🚀 |
|
Thank you @qazxcdswe123 |
Originally posted by @alamb in #15016 (comment)
removed all API that are deprecated since
version <= 40