-
Notifications
You must be signed in to change notification settings - Fork 261
fix!: replace ProofExpr::get_column_references with ProofExpr::get_column_fields
#1087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1087 +/- ##
==========================================
- Coverage 96.72% 96.69% -0.04%
==========================================
Files 302 302
Lines 58840 58909 +69
==========================================
+ Hits 56915 56961 +46
- Misses 1925 1948 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the ProofExpr trait to replace the get_column_references method with get_column_fields, addressing a hidden bug where column references only make sense when they're on source tables. The change separates the collection of column metadata (ColumnField) from the association with specific table references (ColumnRef).
Key changes:
- Renamed
ProofExpr::get_column_referencestoProofExpr::get_column_fieldswith updated signature - Updated all expression implementations to collect
ColumnFieldinstead ofColumnRef - Modified
LegacyFilterExecandGroupByExecto convertColumnFieldtoColumnRefusing table context
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
proof_expr.rs |
Updated trait method from get_column_references to get_column_fields, changing return type from ColumnRef to ColumnField with updated documentation |
column_expr.rs |
Updated to use get_column_fields and insert ColumnField objects via get_column_field() helper |
add_expr.rs, subtract_expr.rs, multiply_expr.rs |
Updated binary arithmetic expressions to use get_column_fields |
and_expr.rs, or_expr.rs, not_expr.rs |
Updated logical expressions to use get_column_fields |
equals_expr.rs, inequality_expr.rs |
Updated comparison expressions to use get_column_fields |
cast_expr.rs, scaling_cast_expr.rs |
Updated cast expressions to use get_column_fields |
literal_expr.rs, placeholder_expr.rs |
Updated leaf expressions with empty get_column_fields implementations |
dyn_proof_expr.rs |
Added ColumnField to imports |
legacy_filter_exec.rs |
Updated to collect ColumnFields and convert to ColumnRefs using table reference |
group_by_exec.rs |
Updated to collect ColumnFields and convert to ColumnRefs using table reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cb64b39 to
3c9d4e1
Compare
|
This PR has been marked as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs. |
Rationale for this change
We have a hidden bug, namely
ProofExpr::get_column_referencesonly makes sense if they are on a source table since inquery_proof.rsthat's how they are eventually used. Hence nothing other thanTableExec,LegacyFilterExecandGroupByExecshould actually be sources of these column references. In order to achieve that the first step is that we only allow deprecated proof plans,LegacyFilterExecandGroupByExec, to useColumnFields ofProofExprs they use since these are actually reliably on source tables. Eventuallyget_column_referencesis scheduled to be removed everywhere since what datafusion and the planner consider to be inTableExecs are precisely the columns of source tables we need.