comphead commented on code in PR #7084:
URL: https://github.com/apache/datafusion-comet/pull/7084#discussion_r4329263662
##########
native/spark-expr/src/hash_funcs/murmur3.rs:
##########
@@ -774,6 +774,58 @@ mod tests {
);
}
+ /// Spark hashes `YearMonthIntervalType` as an int and
`DayTimeIntervalType` as a long, which
+ /// Comet maps to `Interval(YearMonth)` and `Duration(Microsecond)`.
+ #[test]
+ fn test_ansi_intervals_hash_like_int_and_long() {
+ use arrow::array::{DurationMicrosecondArray, IntervalYearMonthArray};
+
+ test_murmur3_hash::<i32, IntervalYearMonthArray>(
+ vec![Some(1), Some(0), Some(-1), Some(i32::MAX), Some(i32::MIN)],
+ vec![0xdea578e3, 0x379fae8f, 0xa0590e3d, 0x07fb67e7, 0x2b1f0fc6],
+ );
+ test_murmur3_hash::<i64, DurationMicrosecondArray>(
+ vec![Some(1), Some(0), Some(-1), Some(i64::MAX), Some(i64::MIN)],
+ vec![0x99f0149d, 0x9c67b85d, 0xc8008529, 0xa05b5d7b, 0xcd1e64fb],
+ );
+ }
+
+ /// A `NullType` value is always null, so it leaves the hash unchanged, as
in Spark, both as a
+ /// column of its own and as a struct field, including a field of a null
struct.
+ #[test]
+ fn test_null_type_leaves_hash_unchanged() {
Review Comment:
I could not find a test that hashes an array whose element type is
`NullType`, for example a `ListArray` over a `NullArray`. In
`hash_list_with_primitive_elements!` that type has no arm and goes through the
`_` recursive path, so it relies on the new `Null` arm in
`create_hashes_internal!` being reached from there. Could one case pin it, here
and in `xxhash64.rs`? The same applies to a list of the two interval types. I
did not run it.
##########
native/common/src/struct_nulls.rs:
##########
@@ -49,6 +50,8 @@ pub fn child_with_parent_nulls(
) -> Result<ArrayRef, ArrowError> {
let child = struct_array.column(ordinal);
match struct_array.nulls() {
+ // A `NullArray` is already null at every row and cannot carry a null
buffer.
+ _ if child.data_type() == &DataType::Null => Ok(Arc::clone(child)),
Review Comment:
This arm returns `Ok(Arc::clone(child))`, the same value as the final `_`
arm, and as the first arm it runs the `child.data_type() == &DataType::Null`
comparison on every call, including the common one where the parent has no
nulls. The doc comment on the function does not mention this case. Folding it
into the second arm as `Some(parent) if parent.null_count() > 0 &&
child.data_type() != &DataType::Null` gives one fall-through path, keeps the
check off the no-null path, and keeps the doc comment accurate.
##########
spark/src/test/resources/sql-tests/operators/round_robin_null_and_intervals.sql:
##########
@@ -0,0 +1,43 @@
+-- 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.
+
+-- Config: spark.comet.exec.localTableScan.enabled=true
Review Comment:
Every query here reads `test_rr_null_intervals`, a Parquet table, so I could
not find a `LocalTableScan` in these plans for
`spark.comet.exec.localTableScan.enabled=true` to act on.
`make_time_shuffle.sql`, which also repartitions a Parquet table with native
round-robin, does not set it. Is it needed here? I did not run the file without
it. `positional.enabled=false` on line 21 is already the default in
`CometConf`, so a short comment saying it is pinned on purpose would help.
--
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]
