sunchao commented on code in PR #6170: URL: https://github.com/apache/datafusion-comet/pull/6170#discussion_r4126091403
########## native/spark-expr/src/array_funcs/array_extrema.rs: ########## @@ -0,0 +1,440 @@ +// 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 std::cmp::Ordering; +use std::sync::Arc; + +use arrow::array::{ + make_array, make_comparator, new_empty_array, Array, ArrayAccessor, ArrayRef, AsArray, + DynComparator, ListArray, MutableArrayData, PrimitiveArray, PrimitiveBuilder, StringArrayType, + StructArray, UInt32Array, +}; +use arrow::buffer::NullBuffer; +use arrow::compute::{take, SortOptions}; +use arrow::datatypes::{ArrowPrimitiveType, DataType, Float32Type, Float64Type}; +use datafusion::common::{exec_err, Result, ScalarValue}; +use datafusion::functions_nested::min_max::{array_max_udf, array_min_udf}; +use datafusion::logical_expr::{ + ColumnarValue, ScalarFunctionArgs, ScalarUDF, ScalarUDFImpl, Signature, +}; +use num::Float; + +#[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)] +enum Utf8Collation { + Binary, + BinaryRtrim, + Lcase, + LcaseRtrim, +} + +impl Utf8Collation { + fn compare(self, mut left: &str, mut right: &str, unicode_version: u32) -> Ordering { + if matches!(self, Self::BinaryRtrim | Self::LcaseRtrim) { + // Spark RTRIM ignores trailing U+0020, not arbitrary Unicode whitespace. + left = left.trim_end_matches(' '); + right = right.trim_end_matches(' '); + } + if matches!(self, Self::Binary | Self::BinaryRtrim) { + return left.cmp(right); + } + fn lower(value: &str, unicode_version: u32) -> impl Iterator<Item = u32> + '_ { + value.chars().flat_map(move |c| { + let cp = c as u32; + let [first, second] = match cp { + // Spark treats final sigma as ordinary sigma. + 0x3c2 => [0x3c3, 0], + // Unicode 17 adds these mappings to the library's Unicode 16 data. + 0xa7ce | 0xa7d2 | 0xa7d4 if unicode_version == 17 => [cp + 1, 0], + 0x16ea0..=0x16eb8 if unicode_version == 17 => [cp + 0x1b, 0], Review Comment: Agreed. Added an explicit table of all 28 Unicode 17 upper/lower pairs in `utf8_lcase_unicode_17_mappings_are_version_gated`. Each pair is checked in both input orders: equal under Unicode 17, and distinct (with opposite ordering) under Unicode 16. The test data is independent of the implementation's range/match arms, so both omissions you described are covered. This passed with the final source in 341174464. ########## native/proto/src/proto/expr.proto: ########## @@ -540,6 +540,10 @@ message ScalarFunc { repeated Expr args = 2; DataType return_type = 3; bool fail_on_error = 4; + // array_min/max only: collations of every string leaf in the element type, + // visiting array elements and struct fields depth-first. Empty means binary ordering. + repeated string string_collations = 5; Review Comment: Agreed, a dedicated message fits the existing array serializers better. 341174464 adds `ArrayExtrema` with the child, min/max mode, string-leaf collations, and Unicode version. All native extrema now use it; the extrema-only fields on `ScalarFunc` and the planner's function-name special case are removed. Added Scala serialization coverage and a native planner regression for min/max and binary/LCASE behavior. The focused Spark 4.1 JNI/serialization tests passed with the final library. ########## spark/src/test/resources/sql-tests/expressions/array/array_extrema_collation.sql: ########## @@ -0,0 +1,74 @@ +-- 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. + +-- MinSparkVersion: 4.0 +-- Config: spark.comet.expression.ArrayMin.allowIncompatible=false +-- Config: spark.comet.expression.ArrayMax.allowIncompatible=false +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true + +statement +CREATE TABLE test_array_extrema_collation( + id int, a string, b string, x double, y double) USING parquet + +statement +INSERT INTO test_array_extrema_collation VALUES + (1, 'a', 'B', double('0.0'), double('-0.0')), + (2, 'B', 'a', double('-0.0'), double('0.0')), + (3, 'A', 'a', double('-0.0'), double('0.0')), + (4, NULL, 'B', NULL, double('0.0')), + (5, NULL, NULL, NULL, NULL), + (6, 'x ', 'x', 1.0, 2.0) Review Comment: Added all three pairs in both input orders to the SQL fixture in 341174464: U+0130 versus i + U+0307, final sigma versus capital sigma, and Kelvin sign versus k. They run through the existing native-extrema assertions, including the LCASE first-winner cases. The updated fixture passed against Spark 4.1.3 with the final release JNI library; I have not rerun Spark 4.2 locally, so that remains for the requested CI matrix. -- 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]
