Skip to content

Commit

Permalink
chore: remove DataPtr trait since Arc::ptr_eq ignores pointer metadata (
Browse files Browse the repository at this point in the history
#10378)

* chore: remove DataPtr trait since Arc::ptr_eq ignores pointer metadata

* fix: remove DataPtr unit tests
  • Loading branch information
intoraw authored May 6, 2024
1 parent b412dba commit c1f1370
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 55 deletions.
48 changes: 0 additions & 48 deletions datafusion/common/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,34 +525,6 @@ pub fn list_ndims(data_type: &DataType) -> u64 {
}
}

/// An extension trait for smart pointers. Provides an interface to get a
/// raw pointer to the data (with metadata stripped away).
///
/// This is useful to see if two smart pointers point to the same allocation.
pub trait DataPtr {
/// Returns a raw pointer to the data, stripping away all metadata.
fn data_ptr(this: &Self) -> *const ();

/// Check if two pointers point to the same data.
fn data_ptr_eq(this: &Self, other: &Self) -> bool {
// Discard pointer metadata (including the v-table).
let this = Self::data_ptr(this);
let other = Self::data_ptr(other);

std::ptr::eq(this, other)
}
}

// Currently, it's brittle to compare `Arc`s of dyn traits with `Arc::ptr_eq`
// due to this check including v-table equality. It may be possible to use
// `Arc::ptr_eq` directly if a fix to https://github.com/rust-lang/rust/issues/103763
// is stabilized.
impl<T: ?Sized> DataPtr for Arc<T> {
fn data_ptr(this: &Self) -> *const () {
Arc::as_ptr(this) as *const ()
}
}

/// Adopted from strsim-rs for string similarity metrics
pub mod datafusion_strsim {
// Source: https://github.com/dguo/strsim-rs/blob/master/src/lib.rs
Expand Down Expand Up @@ -974,26 +946,6 @@ mod tests {
assert_eq!(longest_consecutive_prefix([1, 2, 3, 4]), 0);
}

#[test]
fn arc_data_ptr_eq() {
let x = Arc::new(());
let y = Arc::new(());
let y_clone = Arc::clone(&y);

assert!(
Arc::data_ptr_eq(&x, &x),
"same `Arc`s should point to same data"
);
assert!(
!Arc::data_ptr_eq(&x, &y),
"different `Arc`s should point to different data"
);
assert!(
Arc::data_ptr_eq(&y, &y_clone),
"cloned `Arc` should point to same data as the original"
);
}

#[test]
fn test_merge_and_order_indices() {
assert_eq!(
Expand Down
5 changes: 2 additions & 3 deletions datafusion/core/src/physical_optimizer/join_selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1544,7 +1544,6 @@ mod hash_join_tests {

use arrow::datatypes::{DataType, Field};
use arrow::record_batch::RecordBatch;
use datafusion_common::utils::DataPtr;

struct TestCase {
case: String,
Expand Down Expand Up @@ -1937,8 +1936,8 @@ mod hash_join_tests {
..
}) = plan.as_any().downcast_ref::<HashJoinExec>()
{
let left_changed = Arc::data_ptr_eq(left, &right_exec);
let right_changed = Arc::data_ptr_eq(right, &left_exec);
let left_changed = Arc::ptr_eq(left, &right_exec);
let right_changed = Arc::ptr_eq(right, &left_exec);
// If this is not equal, we have a bigger problem.
assert_eq!(left_changed, right_changed);
assert_eq!(
Expand Down
3 changes: 1 addition & 2 deletions datafusion/physical-expr-common/src/physical_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ use arrow::array::BooleanArray;
use arrow::compute::filter_record_batch;
use arrow::datatypes::{DataType, Schema};
use arrow::record_batch::RecordBatch;
use datafusion_common::utils::DataPtr;
use datafusion_common::{internal_err, not_impl_err, Result};
use datafusion_expr::interval_arithmetic::Interval;
use datafusion_expr::ColumnarValue;
Expand Down Expand Up @@ -188,7 +187,7 @@ pub fn with_new_children_if_necessary(
|| children
.iter()
.zip(old_children.iter())
.any(|(c1, c2)| !Arc::data_ptr_eq(c1, c2))
.any(|(c1, c2)| !Arc::ptr_eq(c1, c2))
{
Ok(expr.with_new_children(children)?)
} else {
Expand Down
3 changes: 1 addition & 2 deletions datafusion/physical-plan/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ use crate::sorts::sort_preserving_merge::SortPreservingMergeExec;
use arrow::datatypes::SchemaRef;
use arrow::record_batch::RecordBatch;
use datafusion_common::config::ConfigOptions;
use datafusion_common::utils::DataPtr;
use datafusion_common::Result;
use datafusion_execution::TaskContext;
use datafusion_physical_expr::{
Expand Down Expand Up @@ -684,7 +683,7 @@ pub fn with_new_children_if_necessary(
|| children
.iter()
.zip(old_children.iter())
.any(|(c1, c2)| !Arc::data_ptr_eq(c1, c2))
.any(|(c1, c2)| !Arc::ptr_eq(c1, c2))
{
plan.with_new_children(children)
} else {
Expand Down

0 comments on commit c1f1370

Please sign in to comment.