Avoid ConfigOptions in IR nodes#19186
Avoid ConfigOptions in IR nodes#19186rapids-bot[bot] merged 12 commits intorapidsai:branch-25.08from
Conversation
This favors putting simpler objects (e.g. `config_options.executor.max_rows_per_partition`) in IR nodes rather than large, complicated objects. This makes understanding what options actually affect the IR node easier, at the cost of making constructing the objects a bit more complicated.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
github-actions
bot
added
Python
TomAugspurger
added
improvement
|
/ok to test |
rjzamora
left a comment
rjzamora
left a comment
There was a problem hiding this comment.
I think this makes sense. I think my only concern is that Scan.parquet_options feels a bit "too-specific" for Scan maybe?
| parquet_options: ParquetOptions | None | ||
| """Parquet-specific options.""" |
There was a problem hiding this comment.
Do we expect to eventually need json_options and csv_options options as well?
There was a problem hiding this comment.
If and when we expose those as ConfigOptions, then yes, I think so. I think the requirement here is that anything passed into Scan.do_evaluate needs to be present on the node.
If we add those we might want to refactor things into some kind of IOOptions.
Yeah, I think this is a consequence of |
rjzamora
left a comment
rjzamora
left a comment
There was a problem hiding this comment.
This is looking good - Some small comments.
|
/merge |
4 participants
This favors putting simpler objects (e.g.
config_options.executor.max_rows_per_partition) in IR nodes rather than large, complicated objects likeconfig_options. This makes understanding what options actually affect the IR node easier, at the cost of making constructing the objects a bit more complicated.