sunchao commented on code in PR #179: URL: https://github.com/apache/arrow-datafusion-comet/pull/179#discussion_r1523632287
########## core/src/execution/datafusion/expressions/bloom_filter_might_contain.rs: ########## @@ -0,0 +1,157 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::{ + execution::datafusion::util::spark_bloom_filter::SparkBloomFilter, parquet::data_type::AsBytes, +}; +use arrow::record_batch::RecordBatch; +use arrow_array::{cast::as_primitive_array, BooleanArray}; +use arrow_schema::{DataType, Schema}; +use datafusion::physical_plan::ColumnarValue; +use datafusion_common::{internal_err, DataFusionError, Result, ScalarValue}; +use datafusion_physical_expr::{aggregate::utils::down_cast_any_ref, PhysicalExpr}; +use std::{ + any::Any, + fmt::Display, + hash::{Hash, Hasher}, + sync::Arc, +}; + +/// A physical expression that checks if a value might be in a bloom filter. It corresponds to the +/// Spark's `BloomFilterMightContain` expression. + +#[derive(Debug, Hash)] +pub struct BloomFilterMightContain { + pub bloom_filter_expr: Arc<dyn PhysicalExpr>, + pub value_expr: Arc<dyn PhysicalExpr>, + bloom_filter: Option<SparkBloomFilter>, +} + +impl Display for BloomFilterMightContain { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!( + f, + "BloomFilterMightContain [bloom_filter_expr: {}, value_expr: {}]", + self.bloom_filter_expr, self.value_expr + ) + } +} + +impl PartialEq<dyn Any> for BloomFilterMightContain { + fn eq(&self, _other: &dyn Any) -> bool { + down_cast_any_ref(_other) + .downcast_ref::<Self>() + .map(|other| { + self.bloom_filter_expr.eq(&other.bloom_filter_expr) + && self.value_expr.eq(&other.value_expr) + }) + .unwrap_or(false) + } +} + +fn evaluate_bloom_filter( + bloom_filter_expr: &Arc<dyn PhysicalExpr>, +) -> Result<Option<SparkBloomFilter>> { + // bloom_filter_expr must be a literal/scalar subquery expression, so we can evaluate it + // with an empty batch with empty schema + let batch = RecordBatch::new_empty(Arc::new(Schema::empty())); + let bloom_filter_bytes = bloom_filter_expr.evaluate(&batch)?; + match bloom_filter_bytes { + ColumnarValue::Scalar(ScalarValue::Binary(v)) => { + Ok(v.map(|v| SparkBloomFilter::new_from_buf(v.as_bytes()))) + } + _ => internal_err!("Bloom filter expression must be evaluated as a scalar binary value"), + } +} + +impl BloomFilterMightContain { + pub fn new( + bloom_filter_expr: Arc<dyn PhysicalExpr>, + value_expr: Arc<dyn PhysicalExpr>, + ) -> Self { + // early evaluate the bloom_filter_expr to get the actual bloom filter + let bloom_filter = evaluate_bloom_filter(&bloom_filter_expr) + .expect("bloom_filter_expr could be evaluated statically"); Review Comment: "could be" -> "could not be"? also we can consider returning `Result` and change this to `try_new`, but not a big deal. ########## core/src/execution/datafusion/util/spark_bloom_filter.rs: ########## @@ -0,0 +1,98 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::execution::datafusion::{ + spark_hash::spark_compatible_murmur3_hash, util::spark_bit_array::SparkBitArray, +}; +use arrow_array::{ArrowNativeTypeOp, BooleanArray, Int64Array}; + +const SPARK_BLOOM_FILTER_VERSION_1: i32 = 1; + +/// A Bloom filter implementation that simulates the behavior of Spark's BloomFilter. +/// It's not a complete implementation of Spark's BloomFilter, but just add the minimum +/// methods to support mightContainsLong in the native side. + +#[derive(Debug, Hash)] +pub struct SparkBloomFilter { + bits: SparkBitArray, + num_hashes: u32, Review Comment: nit: better add a comment on this, since it is actually the number of hash functions ########## core/src/execution/datafusion/util/spark_bloom_filter.rs: ########## @@ -0,0 +1,98 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::execution::datafusion::{ + spark_hash::spark_compatible_murmur3_hash, util::spark_bit_array::SparkBitArray, +}; +use arrow_array::{ArrowNativeTypeOp, BooleanArray, Int64Array}; + +const SPARK_BLOOM_FILTER_VERSION_1: i32 = 1; + +/// A Bloom filter implementation that simulates the behavior of Spark's BloomFilter. +/// It's not a complete implementation of Spark's BloomFilter, but just add the minimum +/// methods to support mightContainsLong in the native side. + +#[derive(Debug, Hash)] +pub struct SparkBloomFilter { + bits: SparkBitArray, + num_hashes: u32, +} + +impl SparkBloomFilter { + pub fn new_from_buf(buf: &[u8]) -> Self { + let mut offset = 0; + let version = read_num_be_bytes!(i32, 4, buf[offset..]); + offset += 4; + assert_eq!( + version, SPARK_BLOOM_FILTER_VERSION_1, + "Unsupported BloomFilter version" + ); + let num_hashes = read_num_be_bytes!(i32, 4, buf[offset..]); + offset += 4; + let num_words = read_num_be_bytes!(i32, 4, buf[offset..]); + offset += 4; + let mut bits = vec![0u64; num_words as usize]; + for i in 0..num_words { + bits[i as usize] = read_num_be_bytes!(i64, 8, buf[offset..]) as u64; + offset += 8; + } + Self { + bits: SparkBitArray::new(bits), + num_hashes: num_hashes as u32, + } + } + + pub fn put_long(&mut self, item: i64) -> bool { Review Comment: I think this is not used right now but perhaps it would be in future? ########## spark/src/main/scala/org/apache/comet/shims/ShimQueryPlanSerde.scala: ########## @@ -44,4 +44,9 @@ trait ShimQueryPlanSerde { failOnError.head } } + + // todo: delete after drop Spark 3.2 support Review Comment: nit: `todo` -> `TODO` ########## core/src/execution/datafusion/util/spark_bloom_filter.rs: ########## @@ -0,0 +1,98 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::execution::datafusion::{ + spark_hash::spark_compatible_murmur3_hash, util::spark_bit_array::SparkBitArray, +}; +use arrow_array::{ArrowNativeTypeOp, BooleanArray, Int64Array}; + +const SPARK_BLOOM_FILTER_VERSION_1: i32 = 1; + +/// A Bloom filter implementation that simulates the behavior of Spark's BloomFilter. +/// It's not a complete implementation of Spark's BloomFilter, but just add the minimum +/// methods to support mightContainsLong in the native side. + +#[derive(Debug, Hash)] +pub struct SparkBloomFilter { + bits: SparkBitArray, + num_hashes: u32, +} + +impl SparkBloomFilter { + pub fn new_from_buf(buf: &[u8]) -> Self { Review Comment: nit: I think we can just call this `new`? -- 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]
