andygrove commented on code in PR #5150: URL: https://github.com/apache/datafusion-comet/pull/5150#discussion_r3693361681
########## native/spark-expr/src/conversion_funcs/trim.rs: ########## @@ -0,0 +1,196 @@ +// 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. + +//! Whitespace trimming for parsing from string. +//! +//! Spark's string casts do not all agree on what "whitespace" means. There are exactly two +//! regimes, and neither of them matches Rust's `str::trim` (which trims Unicode whitespace) or +//! `<[u8]>::trim_ascii` (which omits `0x0B`): +//! +//! | Regime | Trimmed bytes | Cast targets | +//! |----------------------|--------------------------|-------------------------------------------------------| +//! | [`trim_all`] | `0x00`-`0x20` and `0x7F` | boolean, byte, short, int, long, date, timestamp \* | +//! | [`trim_java_string`] | `0x00`-`0x20` | float, double, decimal | +//! +//! Crucially, **neither regime trims any non-ASCII whitespace**. `U+0085`, `U+00A0`, `U+1680`, +//! `U+2000`-`U+200A`, `U+2028`, `U+2029`, `U+202F`, `U+205F` and `U+3000` all leave Spark +//! returning NULL (or raising under ANSI) for every cast target, so using `str::trim` here +//! silently produces a value where Spark produces none. +//! +//! The two regimes differ only in `0x7F` (DELETE), which the `trimAll` set removes and the +//! `String.trim` set does not. That single byte is why a shared helper cannot be applied +//! uniformly: trimming it in the float/double/decimal paths would introduce a new divergence. +//! +//! \* `timestamp` and `timestamp_ntz` are listed for what Spark does; the Comet parsers for +//! those two targets still use `str::trim` and have not been migrated to these helpers Review Comment: Addressed in 434efec — sorry for answering this only in a summary comment rather than here. The `trim.rs` module doc keeps the regime table and the footnote; both paragraphs that restated the table are gone, replaced by one line. `is_whitespace_or_iso_control`, `trim_all` and `trim_java_string` are now pointers at that table rather than re-listing byte ranges and cast targets, so the mapping has a single place to go stale. Same treatment for `trimPadding`'s Scaladoc in `CometCastSuite`. I kept only the two things not derivable from the code: the signed-byte widening note, and the JDK call paths (`Double.parseDouble` / `Decimal.stringToJavaBigDecimal`) that justify the `String.trim` regime — the latter moved into the module doc so it is not duplicated in a function doc. ########## native/spark-expr/src/conversion_funcs/trim.rs: ########## @@ -0,0 +1,196 @@ +// 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. + +//! Whitespace trimming for parsing from string. +//! +//! Spark's string casts do not all agree on what "whitespace" means. There are exactly two +//! regimes, and neither of them matches Rust's `str::trim` (which trims Unicode whitespace) or +//! `<[u8]>::trim_ascii` (which omits `0x0B`): +//! +//! | Regime | Trimmed bytes | Cast targets | +//! |----------------------|--------------------------|-------------------------------------------------------| +//! | [`trim_all`] | `0x00`-`0x20` and `0x7F` | boolean, byte, short, int, long, date, timestamp \* | +//! | [`trim_java_string`] | `0x00`-`0x20` | float, double, decimal | +//! +//! Crucially, **neither regime trims any non-ASCII whitespace**. `U+0085`, `U+00A0`, `U+1680`, +//! `U+2000`-`U+200A`, `U+2028`, `U+2029`, `U+202F`, `U+205F` and `U+3000` all leave Spark +//! returning NULL (or raising under ANSI) for every cast target, so using `str::trim` here +//! silently produces a value where Spark produces none. +//! +//! The two regimes differ only in `0x7F` (DELETE), which the `trimAll` set removes and the +//! `String.trim` set does not. That single byte is why a shared helper cannot be applied +//! uniformly: trimming it in the float/double/decimal paths would introduce a new divergence. Review Comment: Fixed in 434efec: both duplicated paragraphs (the non-ASCII-whitespace one and the regime restatement) are gone, replaced by a single line, with the regime table and footnote left as the one authoritative place. ########## native/spark-expr/src/conversion_funcs/trim.rs: ########## @@ -0,0 +1,196 @@ +// 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. + +//! Whitespace trimming for parsing from string. +//! +//! Spark's string casts do not all agree on what "whitespace" means. There are exactly two +//! regimes, and neither of them matches Rust's `str::trim` (which trims Unicode whitespace) or +//! `<[u8]>::trim_ascii` (which omits `0x0B`): +//! +//! | Regime | Trimmed bytes | Cast targets | +//! |----------------------|--------------------------|-------------------------------------------------------| +//! | [`trim_all`] | `0x00`-`0x20` and `0x7F` | boolean, byte, short, int, long, date, timestamp \* | +//! | [`trim_java_string`] | `0x00`-`0x20` | float, double, decimal | +//! +//! Crucially, **neither regime trims any non-ASCII whitespace**. `U+0085`, `U+00A0`, `U+1680`, +//! `U+2000`-`U+200A`, `U+2028`, `U+2029`, `U+202F`, `U+205F` and `U+3000` all leave Spark +//! returning NULL (or raising under ANSI) for every cast target, so using `str::trim` here +//! silently produces a value where Spark produces none. +//! +//! The two regimes differ only in `0x7F` (DELETE), which the `trimAll` set removes and the +//! `String.trim` set does not. That single byte is why a shared helper cannot be applied +//! uniformly: trimming it in the float/double/decimal paths would introduce a new divergence. +//! +//! \* `timestamp` and `timestamp_ntz` are listed for what Spark does; the Comet parsers for +//! those two targets still use `str::trim` and have not been migrated to these helpers +//! (<https://github.com/apache/datafusion-comet/issues/5149>). + +/// True for the bytes trimmed by `org.apache.spark.unsafe.types.UTF8String.trimAll`, i.e. the +/// bytes `b` for which `Character.isWhitespace(b) || Character.isISOControl(b)` holds. +/// +/// `isWhitespace` covers `0x09`-`0x0D`, `0x1C`-`0x1F` and `0x20`; `isISOControl` covers +/// `0x00`-`0x1F` and `0x7F`; the union is `0x00`-`0x20` plus `0x7F`. Spark widens a *signed* +/// `byte` into the `int` overload, so bytes `0x80`-`0xFF` arrive negative and are never trimmed. Review Comment: Fixed in 434efec: `is_whitespace_or_iso_control`'s doc no longer spells out either byte range or their union. It now points at the module table, with only the signed-byte widening note kept, since that part is not obvious from the expression. ########## native/spark-expr/src/conversion_funcs/trim.rs: ########## @@ -0,0 +1,196 @@ +// 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. + +//! Whitespace trimming for parsing from string. +//! +//! Spark's string casts do not all agree on what "whitespace" means. There are exactly two +//! regimes, and neither of them matches Rust's `str::trim` (which trims Unicode whitespace) or +//! `<[u8]>::trim_ascii` (which omits `0x0B`): +//! +//! | Regime | Trimmed bytes | Cast targets | +//! |----------------------|--------------------------|-------------------------------------------------------| +//! | [`trim_all`] | `0x00`-`0x20` and `0x7F` | boolean, byte, short, int, long, date, timestamp \* | +//! | [`trim_java_string`] | `0x00`-`0x20` | float, double, decimal | +//! +//! Crucially, **neither regime trims any non-ASCII whitespace**. `U+0085`, `U+00A0`, `U+1680`, +//! `U+2000`-`U+200A`, `U+2028`, `U+2029`, `U+202F`, `U+205F` and `U+3000` all leave Spark +//! returning NULL (or raising under ANSI) for every cast target, so using `str::trim` here +//! silently produces a value where Spark produces none. +//! +//! The two regimes differ only in `0x7F` (DELETE), which the `trimAll` set removes and the +//! `String.trim` set does not. That single byte is why a shared helper cannot be applied +//! uniformly: trimming it in the float/double/decimal paths would introduce a new divergence. +//! +//! \* `timestamp` and `timestamp_ntz` are listed for what Spark does; the Comet parsers for +//! those two targets still use `str::trim` and have not been migrated to these helpers +//! (<https://github.com/apache/datafusion-comet/issues/5149>). + +/// True for the bytes trimmed by `org.apache.spark.unsafe.types.UTF8String.trimAll`, i.e. the +/// bytes `b` for which `Character.isWhitespace(b) || Character.isISOControl(b)` holds. +/// +/// `isWhitespace` covers `0x09`-`0x0D`, `0x1C`-`0x1F` and `0x20`; `isISOControl` covers +/// `0x00`-`0x1F` and `0x7F`; the union is `0x00`-`0x20` plus `0x7F`. Spark widens a *signed* +/// `byte` into the `int` overload, so bytes `0x80`-`0xFF` arrive negative and are never trimmed. +#[inline] +const fn is_whitespace_or_iso_control(b: u8) -> bool { + b <= 0x20 || b == 0x7F +} + +/// True for the bytes trimmed by `java.lang.String.trim`, which drops any char `<= U+0020`. +/// +/// A char above `U+0020` always encodes to bytes `>= 0x80` in UTF-8, so testing bytes rather +/// than chars gives the same answer. +#[inline] +const fn is_java_trim_byte(b: u8) -> bool { + b <= 0x20 +} + +/// Trims the `UTF8String.trimAll` byte set (`0x00`-`0x20` and `0x7F`) from both ends. +/// +/// This is the trim used by `CAST(string AS boolean)`, the integral casts and `date_parser`. +/// See the [module docs](self) for why the other targets need [`trim_java_string`]. Review Comment: Fixed in 434efec — `trim_all`'s doc is now just: ``` /// See the [module docs](self) for which cast targets use this regime and which use /// [`trim_java_string`]. ``` ########## native/spark-expr/src/conversion_funcs/trim.rs: ########## @@ -0,0 +1,196 @@ +// 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. + +//! Whitespace trimming for parsing from string. +//! +//! Spark's string casts do not all agree on what "whitespace" means. There are exactly two +//! regimes, and neither of them matches Rust's `str::trim` (which trims Unicode whitespace) or +//! `<[u8]>::trim_ascii` (which omits `0x0B`): +//! +//! | Regime | Trimmed bytes | Cast targets | +//! |----------------------|--------------------------|-------------------------------------------------------| +//! | [`trim_all`] | `0x00`-`0x20` and `0x7F` | boolean, byte, short, int, long, date, timestamp \* | +//! | [`trim_java_string`] | `0x00`-`0x20` | float, double, decimal | +//! +//! Crucially, **neither regime trims any non-ASCII whitespace**. `U+0085`, `U+00A0`, `U+1680`, +//! `U+2000`-`U+200A`, `U+2028`, `U+2029`, `U+202F`, `U+205F` and `U+3000` all leave Spark +//! returning NULL (or raising under ANSI) for every cast target, so using `str::trim` here +//! silently produces a value where Spark produces none. +//! +//! The two regimes differ only in `0x7F` (DELETE), which the `trimAll` set removes and the +//! `String.trim` set does not. That single byte is why a shared helper cannot be applied +//! uniformly: trimming it in the float/double/decimal paths would introduce a new divergence. +//! +//! \* `timestamp` and `timestamp_ntz` are listed for what Spark does; the Comet parsers for +//! those two targets still use `str::trim` and have not been migrated to these helpers +//! (<https://github.com/apache/datafusion-comet/issues/5149>). + +/// True for the bytes trimmed by `org.apache.spark.unsafe.types.UTF8String.trimAll`, i.e. the +/// bytes `b` for which `Character.isWhitespace(b) || Character.isISOControl(b)` holds. +/// +/// `isWhitespace` covers `0x09`-`0x0D`, `0x1C`-`0x1F` and `0x20`; `isISOControl` covers +/// `0x00`-`0x1F` and `0x7F`; the union is `0x00`-`0x20` plus `0x7F`. Spark widens a *signed* +/// `byte` into the `int` overload, so bytes `0x80`-`0xFF` arrive negative and are never trimmed. +#[inline] +const fn is_whitespace_or_iso_control(b: u8) -> bool { + b <= 0x20 || b == 0x7F +} + +/// True for the bytes trimmed by `java.lang.String.trim`, which drops any char `<= U+0020`. +/// +/// A char above `U+0020` always encodes to bytes `>= 0x80` in UTF-8, so testing bytes rather +/// than chars gives the same answer. +#[inline] +const fn is_java_trim_byte(b: u8) -> bool { + b <= 0x20 +} + +/// Trims the `UTF8String.trimAll` byte set (`0x00`-`0x20` and `0x7F`) from both ends. +/// +/// This is the trim used by `CAST(string AS boolean)`, the integral casts and `date_parser`. +/// See the [module docs](self) for why the other targets need [`trim_java_string`]. +#[inline] +pub(crate) fn trim_all(s: &str) -> &str { + let (start, end) = trim_all_range(s.as_bytes()); + &s[start..end] +} + +/// [`trim_all`] over a byte slice, for parsers that already work on bytes. +#[inline] +pub(crate) fn trim_all_bytes(bytes: &[u8]) -> &[u8] { + trim_bytes(bytes, is_whitespace_or_iso_control).0 +} + +/// The byte offsets [`trim_all`] would slice at, for parsers that need to keep a cursor into +/// the untrimmed input. +#[inline] +pub(crate) fn trim_all_range(bytes: &[u8]) -> (usize, usize) { + let (trimmed, start) = trim_bytes(bytes, is_whitespace_or_iso_control); + (start, start + trimmed.len()) +} + +/// Trims the `java.lang.String.trim` byte set (`0x00`-`0x20`, keeping `0x7F`) from both ends. +/// +/// This is the trim used by `CAST(string AS float/double)` (via `Double.parseDouble`, which +/// calls `String.trim` before parsing) and by `CAST(string AS decimal)` (via +/// `Decimal.stringToJavaBigDecimal`, which does `str.toString.trim`). Review Comment: Fixed in 434efec the same way: `trim_java_string`'s doc points at the module table instead of re-listing float/double/decimal. The JDK call paths moved up into the module doc, so they are stated once rather than in both places. ########## spark/src/test/scala/org/apache/comet/CometCastSuite.scala: ########## @@ -811,6 +811,43 @@ class CometCastSuite extends CometTestBase with AdaptiveSparkPlanHelper { castTest(testValues, DataTypes.BooleanType) } + /** + * Padding used to check that Comet trims exactly the byte set that each Spark cast trims. Spark + * has two trim regimes and neither of them trims any non-ASCII whitespace: + * - `UTF8String.trimAll` (bytes `0x00`-`0x20` and `0x7F`) for boolean, integral and datetime + * - `java.lang.String.trim` (bytes `0x00`-`0x20` only) for float, double and decimal + * + * Spark itself is the oracle here, so the expectations do not need to be spelled out. See + * https://github.com/apache/datafusion-comet/issues/5149. Review Comment: Fixed in 434efec: `trimPadding`'s Scaladoc now points at `conversion_funcs::trim` instead of re-deriving both regimes in Scala, so the Rust table is the only copy. -- 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]
