fix: ParquetSource - with_predicate() don't have to reset metrics
#17858
Conversation
| self | ||
| } | ||
|
|
||
| fn with_metrics(mut self, metrics: ExecutionPlanMetricsSet) -> Self { |
There was a problem hiding this comment.
Is this removal intentional ?
It is not related to with_predicate()
There was a problem hiding this comment.
Yes, with_predicate() is the only place calling this deleted function, and now it's removed
There was a problem hiding this comment.
Right!
with_metrics() was private!
But there is no setter for metrics now... How is it supposed to be used ? Use directly the field ?!
| } | ||
|
|
||
| /// Set predicate information | ||
| pub fn with_predicate(&self, predicate: Arc<dyn PhysicalExpr>) -> Self { |
There was a problem hiding this comment.
This method is a bit strange.
The other with_**() methods accept mut self and modify in-place. Only this method accepts &self and makes a clone of itself.
There was a problem hiding this comment.
I agree it would be nice to figure out why there is this discrepancy
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @2010YOUY01 and @martin-g
| } | ||
|
|
||
| /// Set predicate information | ||
| pub fn with_predicate(&self, predicate: Arc<dyn PhysicalExpr>) -> Self { |
There was a problem hiding this comment.
I agree it would be nice to figure out why there is this discrepancy
|
Thanks again @2010YOUY01 and @martin-g |
3 participants
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
with_predicate()is a builder pattern API to overwritepredicatefield forParquetSource. Now it's also resetting the metrics, this step should be unnecessary.This change was introduced by a giant refactor #14224, so likely this is done by mistake, instead of some certain reasons.
Are these changes tested?
Existing test has covered metrics.
Since now this
with_predicate()is called before constructing metrics set, so it's not introducing any bug, and no test case needs to be changed.Are there any user-facing changes?