Core: Optimize the TableScanContext #5982
Conversation
|
I believe it's better to move the construction responsibility of the TableScanContext to the inner Builder. Have the builder perform the create operation instead of new TableScanContext(). |
There was a problem hiding this comment.
looks great, but I would rename the copy method from builder to from as I think this is clearer.
@rdblue / @hililiwei I also created #5985 to have a slightly shorter alternative (171 vs 376 LOC) that doesn't require a custom builder.
| .metricsReporter(context.metricsReporter); | ||
| } | ||
|
|
||
| static Builder builder(TableScanContext context) { |
There was a problem hiding this comment.
I would probably just call this from() rather than build() to make it clearer that this is copying state
There was a problem hiding this comment.
sounds good. Done.
Thanks fro your review. #5985 Annotations is really more concise. |
|
I don't think that this is worth the change. The context doesn't change very often (this is the 4th change in the 2 years since this was introduced) and I don't think that a builder helps much. In addition, adding a builder is another class to maintain and would be passing state around again. |
|
Close with #5985 |
5 participants
This PR optimizes the
TableScanContextto make it easier to read and modify.Before this PR, if you want to add a field, you need to modify a lot.