kumarUjjawal commented on code in PR #24165:
URL: https://github.com/apache/datafusion/pull/24165#discussion_r3737381924
##########
datafusion/proto-models/src/generated/pbjson.rs:
##########
@@ -9063,6 +9066,11 @@ impl serde::Serialize for HashJoinExecNode {
if let Some(v) = self.dynamic_filter.as_ref() {
struct_ser.serialize_field("dynamicFilter", v)?;
}
+ if let Some(v) = self.fetch.as_ref() {
Review Comment:
Binary protobuf compatibility is correct, but JSON is not forward
compatible. An older JSON reader rejects the new fetch field?
##########
datafusion/proto-models/src/generated/prost.rs:
##########
@@ -2029,6 +2029,15 @@ pub struct HashJoinExecNode {
/// Optional dynamic filter expression for pushing down to the probe side.
#[prost(message, optional, tag = "11")]
pub dynamic_filter: ::core::option::Option<PhysicalExprNode>,
+ /// Optional row limit pushed into the join by the `limit_pushdown` rule.
+ ///
+ /// This is presence-tracked (`optional`) on purpose: messages produced by
+ /// versions predating this field carry no `fetch` at all, and a plain
proto3
+ /// scalar would decode that absence as `0`, i.e. "fetch 0 rows", silently
+ /// turning old plans into empty results. With `optional`, absent decodes
to
+ /// `None`, which is the correct reading of an older message.
+ #[prost(uint64, optional, tag = "12")]
+ pub fetch: ::core::option::Option<u64>,
Review Comment:
will Adding fetch breaks downstream exhaustive HashJoinExecNode struct
literals?
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1932,17 +1933,18 @@ impl HashJoinExec {
indices => Some(indices.iter().map(|i| *i as usize).collect()),
};
- let mut hash_join = HashJoinExec::try_new(
- left,
- right,
- on,
- filter,
- &join_type,
- projection,
- partition_mode,
- null_equality,
- hashjoin.null_aware,
- )?;
+ let mut hash_join = HashJoinExecBuilder::new(left, right, on,
join_type)
+ .with_filter(filter)
+ .with_projection(projection)
+ .with_partition_mode(partition_mode)
+ .with_null_equality(null_equality)
+ .with_null_aware(hashjoin.null_aware)
+ // Restore the row limit that `limit_pushdown` may have pushed into
+ // the join. The field is presence-tracked, so a message written
+ // before it existed decodes to `None` (no limit) rather than to
+ // `Some(0)`.
+ .with_fetch(hashjoin.fetch.map(|f| f as usize))
Review Comment:
u64 as usize will silently truncates on 32-bit targets. A fetch of 1 << 32
will becomes 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
