Skip to content

fix(proto): correctly serialize FilterExec empty projection #21873

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.

Sign up for GitHub

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

Closed
Adez017 wants to merge 11 commits into from
Closed

fix(proto): correctly serialize FilterExec empty projection #21873

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
833942d
Update mod.rs
Adez017 Apr 27, 2026
a7e8657
cargo fix
Adez017 Apr 27, 2026
59bac42
Merge branch 'main' into patch-1
Adez017 Apr 28, 2026
9a66177
quick fix
Adez017 Apr 28, 2026
f7ba77e
Merge branch 'apache:main' into patch-1
Adez017 Apr 28, 2026
1a28967
test for filterexec
Adez017 Apr 28, 2026
bfc0235
spell fix
Adez017 Apr 28, 2026
3c344be
bracesfix
Adez017 Apr 28, 2026
ceefc02
fmt
Adez017 Apr 28, 2026
44a005b
bug fix
Adez017 Apr 28, 2026
fd8af84
Merge branch 'main' into patch-1
Adez017 Apr 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 13 additions & 11 deletions datafusion/proto/src/physical_plan/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,16 +681,17 @@ impl protobuf::PhysicalPlanNode {
})?;

let filter_selectivity = filter.default_filter_selectivity.try_into();
let projection = if !filter.projection.is_empty() {
Some(
filter
.projection
.iter()
.map(|i| *i as usize)
.collect::<Vec<_>>(),
)
// After deserializing, check if it equals the full range
let num_fields = schema.fields().len();
let full_projection: Vec<usize> = (0..num_fields).collect();
Copy link
Copy Markdown
Contributor

@askalt askalt Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the code requires a vector allocation to check if the other vector is 0,1,2,3.., but we can do it without allocations using a loop (sample code):

let mut projection = Vec::with_capacity();
let mut is_full_projection =
    filter.projection.len() == input.schema().fields.len();
for (i, idx) in filter.projection.iter().enumerate() {
    let idx = idx as usize;
    is_full_projection &= idx == i;
    projection.push(idx);
}
let projection = if is_full_projection {
    None
} else {
    Some(projection)
};

Copy link
Copy Markdown
Contributor Author

@Adez017 Adez017 Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @askalt !

let deserialized: Vec<usize> = filter.projection.iter().map(|i| *i as usize).collect();

let projection = if deserialized == full_projection {
None // treat full range as "no projection"
} else if deserialized.is_empty() {
Some(vec![]) // genuine empty projection
} else {
None
Some(deserialized)
};

let filter = FilterExecBuilder::new(predicate, input)
Expand Down Expand Up @@ -2350,8 +2351,9 @@ impl protobuf::PhysicalPlanNode {
.physical_expr_to_proto(exec.predicate(), codec)?,
),
default_filter_selectivity: exec.default_selectivity() as u32,
projection: exec.projection().as_ref().map_or_else(Vec::new, |v| {
v.iter().map(|x| *x as u32).collect::<Vec<u32>>()
projection: match exec.projection() {
None => (0..fields.len()).map(|i| i as u32).collect(),
Some(v) => v.iter().map(|x| *x as u32).collect(),
}),
batch_size: exec.batch_size() as u32,
fetch: exec.fetch().map(|f| f as u32),
Expand Down
Loading