Repository navigation
datafusion-physical-expr: add DynamicFilterPhysicalExpr::update_from(Self) method - #26534
jayshrivastava wants to merge 6 commits into
Conversation
jayshrivastava
changed the title
DynamicFilterPhysicalExpr::update_from(Self) method
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26534 +/- ##
==========================================
+ Coverage 82.91% 82.94% +0.02%
==========================================
Files 1155 1156 +1
Lines 458058 458717 +659
Branches 458058 458717 +659
==========================================
+ Hits 379813 380489 +676
+ Misses 55202 55148 -54
- Partials 23043 23080 +37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rgbuilds
left a comment
rgbuilds
left a comment
There was a problem hiding this comment.
Re-reviewed the latest changes locally. The completion-semantics concern is resolved, and the focused dynamic-filter, physical-plan, and proto round-trip tests pass, along with formatting and strict Clippy. LGTM.
LiaCastaneda
left a comment
LiaCastaneda
left a comment
There was a problem hiding this comment.
the solution makes sense to me, just had a few questions
| let _ = self.state_watch.send(FilterState::Complete { | ||
| generation: current_generation, | ||
| }); | ||
| self.state_watch.send_replace(current.state()); |
There was a problem hiding this comment.
Is there a reason to change send with send_replace? if its fixing something can we add a test?
There was a problem hiding this comment.
I just didn't like that we were throwing away the error. I figured there's no reason to not use send_replace instead. This way we buffer the update and let potentially late-arriving watchers to read it.
I'm happy to go back to send though if you feel strongly.
|
|
||
| // Release the source lock before acquiring the target lock via clone(). | ||
| let source_state = source.inner.read().clone(); | ||
| if self.children != source.children { |
There was a problem hiding this comment.
Why not just check the ID?
There was a problem hiding this comment.
Yeah that makes sense. The children should not be different. Deleted in favor of just the ID check.
## Which issue does this PR close? - N/A ## Rationale for this change The semver check previously compared the PR against the current state of `apache/main`, and did so by doing a full `git` checkout, including the entire repository history. This approach has two downsides: 1. It is expensive: cloning the entire repo history takes ~9 sec locally, versus ~0.7 for a shallow checkout. 2. There's a race condition which results in comparing the PR against the wrong base when assessing whether the PR breaks semantic versioning. On (2): when a PR is opened or updated, GitHub creates a test merge commit that merges the PR into main as of that moment (T0). Now suppose that there is a delay between the CI jobs being queued and the semver job actually running, so that when the semver job runs, `HEAD` is now at T1. The previous version of this check would compare the PR with T1, not T0. For example, if a commit between T0 and T1 added a new API, the semver check would claim that the PR _removes_ that API. Fix this by comparing the merge commit with its first parent instead, which is T0 in the notation above. ## What changes are included in this PR? See above. ## What is the testing strategy for this PR? CI / dev tooling change only. Tested locally on six open PRs (apache#26565, apache#26000, apache#26582, apache#25874, apache#26534, apache#26576): fetched each PR's merge ref at depth 2 and ran both steps against `HEAD^1`. The crate lists and semver results match CI, including two PRs that branched 66 and 154 commits behind main. ## Are there any user-facing changes? No.
Which issue does this PR close?
What changes are included in this PR?
This PR adds a helper similar to
DynamicFilterPhysicalExpr::update()which takes a differentDynamicFilterPhysicalExpr.Rationale for this change
The use case I have is that I want to update a
Arc<DynamicFilterPhysicalExpr>from a differentArc<DynamicFilterPhysicalExpr>(as opposed to the normalupdate()API which takes a non dynamic filterArc<dyn PhysicalExpr>).This requires accessing private fields like
Inner. Rather than exposing private fields, I thought that adding anupdate_from(Self)API is cleaner.Note that you cannot call
because
a) it doesn't copy
is_completeb) it doesn't copy
inner.exprdirectly.current()returns the expression with remapped columns and callingdynamic_filter_expr.update(remapped_expression); dynamic_filter_expr.current()breaks becausecurrent()cannot remap the remapped expression.Alternatives Considered
inner.exprandis_completepublic. This is undesirable because it adds overheadExecutionPlan. In a way, this is whatupdate_fromdoes, taking advantage of interior mutability.What is the testing strategy for this PR?
One unit test
Are there any user-facing changes?
No.