Skip to content

Commit dedb5cf

Browse files
adriangbclaude
andcommitted
fix(proto): decode HashJoinExec fetch with a checked u64 -> usize
This branch is stacked on the `fetch` fix and, after the rebase onto main, carries its own copy of the `HashJoinExec` decode path. Apply the same correction here so the defect does not survive on this branch and reappear once #24165 merges. Review feedback on #24165: `u64 as usize` silently truncates on a 32-bit target, where `usize` is 32 bits. A fetch of `1 << 32` decodes to `0` -- not merely a wrong limit but the worst possible one, since "fetch 0 rows" turns the query into an empty result instead of erroring. Use `usize::try_from` and surface an out-of-range value as an error. Truncating and saturating both misrepresent the plan; an explicit decode failure is the honest outcome, and it is only reachable on a 32-bit target with an absurd fetch. `plan_datafusion_err!` rather than `internal_datafusion_err!`: this decode path reserves the internal-error macros for genuinely malformed nodes (an unknown `PartitionMode` discriminant, a dynamic filter that does not downcast), which really do indicate a bug. A well-formed but unrepresentable `fetch` is not a DataFusion bug -- it is a plan that cannot be expressed on this target -- and `plan_err!` is already this file's idiom for invalid plan configuration. The encode side (`fetch.map(|f| f as u64)`) needs no change: `usize` is at most 64 bits on every supported target, so widening to `u64` is lossless. `roundtrip_hash_join_fetch` now also covers `u32::MAX as usize` and `usize::MAX`, pinning that a large fetch round-trips exactly. Both are representable on every target, so the test stays portable. The truncation path itself is only reachable on a 32-bit target and is therefore not covered on a 64-bit CI host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 36a7d74 commit dedb5cf

2 files changed

Lines changed: 30 additions & 7 deletions

File tree

‎datafusion/physical-plan/src/joins/hash_join/exec.rs‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1896,7 +1896,7 @@ impl HashJoinExec {
18961896
node: &datafusion_proto_models::protobuf::PhysicalPlanNode,
18971897
ctx: &crate::proto::ExecutionPlanDecodeCtx<'_>,
18981898
) -> Result<Arc<dyn ExecutionPlan>> {
1899-
use datafusion_common::internal_datafusion_err;
1899+
use datafusion_common::{internal_datafusion_err, plan_datafusion_err};
19001900
use datafusion_proto_models::protobuf;
19011901
use std::any::Any;
19021902

@@ -1978,17 +1978,33 @@ impl HashJoinExec {
19781978
indices => Some(indices.iter().map(|i| *i as usize).collect()),
19791979
};
19801980

1981+
// Restore the row limit that `limit_pushdown` may have pushed into the
1982+
// join. The field is presence-tracked, so a message written before it
1983+
// existed decodes to `None` (no limit) rather than to `Some(0)`.
1984+
//
1985+
// The conversion is checked, not `as usize`: `fetch` is a `u64` on the
1986+
// wire but a `usize` in the plan, and on a 32-bit target `as usize`
1987+
// truncates. A fetch of `1 << 32` would become `0` -- not merely a
1988+
// wrong limit but the worst one, silently turning the query into an
1989+
// empty result. Report the out-of-range value instead. Please do not
1990+
// "simplify" this back to `as usize`.
1991+
let fetch = fetch
1992+
.map(|f| {
1993+
usize::try_from(f).map_err(|_| {
1994+
plan_datafusion_err!(
1995+
"HashJoinExec: fetch value {f} cannot be represented as usize on this target"
1996+
)
1997+
})
1998+
})
1999+
.transpose()?;
2000+
19812001
let mut hash_join = HashJoinExecBuilder::new(left, right, on, join_type)
19822002
.with_filter(filter)
19832003
.with_projection(projection)
19842004
.with_partition_mode(partition_mode)
19852005
.with_null_equality(null_equality)
19862006
.with_null_aware(*null_aware)
1987-
// Restore the row limit that `limit_pushdown` may have pushed into
1988-
// the join. The field is presence-tracked, so a message written
1989-
// before it existed decodes to `None` (no limit) rather than to
1990-
// `Some(0)`.
1991-
.with_fetch(fetch.map(|f| f as usize))
2007+
.with_fetch(fetch)
19922008
.build()?;
19932009

19942010
if let Some(dynamic_filter_proto) = dynamic_filter {

‎datafusion/proto/tests/cases/roundtrip_physical_plan.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -563,7 +563,14 @@ fn roundtrip_hash_join_fetch() -> Result<()> {
563563
Arc::new(Column::new("col", schema_right.index_of("col")?)) as _,
564564
)];
565565

566-
for fetch in [None, Some(7)] {
566+
// `usize::MAX` and `u32::MAX as usize` pin the decode-side `u64 -> usize`
567+
// conversion: it is a checked `usize::try_from`, and a large fetch must
568+
// survive the round trip exactly rather than being truncated or clamped.
569+
// Both are representable on every target (on a 32-bit target `usize::MAX`
570+
// is simply `u32::MAX`), so this stays portable. The truncating case
571+
// itself -- a `u64` fetch above `usize::MAX` -- is only reachable on a
572+
// 32-bit target and so is not exercised by this test on a 64-bit host.
573+
for fetch in [None, Some(7), Some(u32::MAX as usize), Some(usize::MAX)] {
567574
let join = HashJoinExec::try_new(
568575
Arc::new(EmptyExec::new(Arc::clone(&schema_left))),
569576
Arc::new(EmptyExec::new(Arc::clone(&schema_right))),

0 commit comments

Comments
 (0)