feat: removed all instances of deprecated virtualtable values field#23672
feat: removed all instances of deprecated virtualtable values field#23672eliot1480 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23672 +/- ##
=======================================
Coverage 80.66% 80.67%
=======================================
Files 1086 1086
Lines 366673 366740 +67
Branches 366673 366740 +67
=======================================
+ Hits 295786 295853 +67
+ Misses 53263 53261 -2
- Partials 17624 17626 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| .await | ||
| } | ||
| Some(ReadType::VirtualTable(vt)) => { | ||
| if vt.values.is_empty() && vt.expressions.is_empty() { |
There was a problem hiding this comment.
I think we can still check and short-circuit on vt.expressions.is_empty() here.
| let mut name_idx = 0; | ||
| for (field_idx, expression) in row.fields.iter().enumerate() { | ||
| let expr = match expression.rex_type.as_ref() { | ||
| Some(substrait::proto::expression::RexType::Literal(lit)) => { |
There was a problem hiding this comment.
Do we actually need to special case the Literal consumer here? I would have assumed that consume_expression would be able to handle it, and more specifically the block in
if !vt.expressions.is_empty() {
had does everything we needed for the conversion here.
Put another way, it looks like you've inlined some of the convert_literal_rows code, but I don't think it's needed.
vbarua
left a comment
There was a problem hiding this comment.
Thinking about this from a migration perspective, I think we need to do ship this as 2 changes:
- Release a change that switches the producer to generate VirtualTables using only the
expressionsfield. - After that is live, release the change that fully removes the old
valuesfield.
Because the producer currently emits plans using the deprecated field, we need to give users a chance to update any stored plans, and then remove support for values.
| // deserialize with field name preservation. | ||
| #[expect(deprecated)] | ||
| values: vec![LiteralStruct { fields }], | ||
| expressions: vec![], |
There was a problem hiding this comment.
Something I'm realizing, for migration purpose we may first want to ship a change that updates this to write these values using expressions.
If we start off right of the bat removing all support for values, anyone storing plans outside of DataFusion won't be able to read them post update, because we'll only handle expressions.
Which issue does this PR close?
Rationale for this change
Since the VirtualTables.values field has been deprecated and is soon to be removed in the next substrait update, we should make sure to delete all instances of it to prevent the presence of dead code.
What changes are included in this PR?
Producer now emits VirtualTable.expressions for Values and EmptyRelation, and the consumer now only reads VirtualTable.expressions. Converted 17 direct virtualTable.values entries across 10 JSON test plans to virtualTable.expressions.
Are these changes tested?
Yes they are. Tests ran:
jq validation for JSON fixtures
Are there any user-facing changes?
No.