ARROW-14725: [C++][Compute] Extract Expression simplification pass registry #11716
Conversation
|
|
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Sorry for the long delay.
This looks good to me, I left a few questions.
|
|
||
| /// The default registry, which includes built-in simplification passes. | ||
| ARROW_EXPORT | ||
| ExpressionSimplificationPassRegistry* default_expression_simplification_registry(); |
There was a problem hiding this comment.
nit: DefaultExpressionSimplificationRegistry?
There was a problem hiding this comment.
Though I see the exec node registry and function registry use different naming schemes (default_exec_factory_registry, GetFunctionRegistry)
| ExpressionSimplificationPassRegistry* default_expression_simplification_registry() { | ||
| class DefaultRegistry : public ExpressionSimplificationPassRegistry { | ||
| public: | ||
| DefaultRegistry() { |
There was a problem hiding this comment.
Looks like FoldConstants should be removed above?
| return expr; | ||
| })); | ||
|
|
||
| if (Identical(simplified, canonicalized)) return expr; |
There was a problem hiding this comment.
Should this return canonicalized?
There was a problem hiding this comment.
It also seems we don't need to canonicalize below before calling this.
|
Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer. |
github-actions
bot
added
the
Status: stale-warning
2 participants
No description provided.