sunchao commented on code in PR #6595: URL: https://github.com/apache/datafusion-comet/pull/6595#discussion_r4192194615
########## spark/src/main/spark-4.x/org/apache/comet/serde/variant.scala: ########## @@ -0,0 +1,115 @@ +/* + * 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. + */ + +package org.apache.comet.serde + +import org.apache.spark.SparkThrowable +import org.apache.spark.sql.catalyst.expressions.{Attribute, AttributeReference, Literal} +import org.apache.spark.sql.catalyst.expressions.variant.{ArrayExtraction, ObjectExtraction, VariantGet} +import org.apache.spark.sql.types._ + +import org.apache.comet.serde.ExprOuterClass.Expr +import org.apache.comet.serde.QueryPlanSerde.serializeDataType + +object CometVariantGet extends CometExpressionSerde[VariantGet] { + private val pathReason = "Variant extraction requires a non-null foldable path." + private val invalidPathReason = + "Invalid Variant paths require Spark's null and evaluation semantics." + private val inputReason = "Variant extraction requires a top-level Variant column or literal." + private val targetReason = + "Variant extraction supports Boolean, numeric, binary, date and timestamp targets only." + private val stringReason = + "Variant STRING extraction requires Spark-compatible JSON and scalar formatting " + + "(https://github.com/apache/datafusion-comet/issues/5424)." + private val decimalReason = + "Floating-point to decimal rounding can differ from Spark on JDK 17 " + + "(https://github.com/apache/datafusion-comet/issues/5424)." + private val temporalReason = + "Date/time parsing and timezone conversion support a narrower year range than Spark " + + "(https://github.com/apache/datafusion-comet/issues/5424)." + + override def getUnsupportedReasons(): Seq[String] = + Seq( + pathReason, + invalidPathReason, + inputReason, + targetReason, + stringReason, + "Timezones with second-resolution offsets cannot be represented in native code.") + override def getIncompatibleReasons(): Seq[String] = Seq(decimalReason, temporalReason) + + override def getSupportLevel(expr: VariantGet): SupportLevel = { + if (!expr.path.foldable || expr.path.eval() == null) { + Unsupported(Some(pathReason)) + } else if (!hasValidPath(expr)) { + Unsupported(Some(invalidPathReason)) + } else if (!(expr.child.isInstanceOf[AttributeReference] || expr.child + .isInstanceOf[Literal]) || + expr.child.dataType != VariantType) { + Unsupported(Some(inputReason)) + } else if (CometTimeZone.nativeId(expr.timeZoneId).isEmpty) { + CometTimeZone.supportLevel(expr.timeZoneId) + } else { + expr.dataType match { + case BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType | + DoubleType | BinaryType => + Compatible() Review Comment: [P2] Reject invalid special-value suffixes before marking floating-point extraction compatible. With `spark.sql.variant.pushVariantIntoScan=false`, store `parse_json('"NaNf"')` in a Parquet Variant column and select `try_variant_get(v, '$', 'double')`. Spark returns NULL, while Comet returns NaN. Strict `variant_get` should raise `INVALID_VARIANT_CAST`, but Comet succeeds. `"Infinityf"` and FLOAT targets behave similarly. The shared parser strips the suffix and accepts the remaining special value. Although that parser behavior predates this PR, newly native Variant extraction exposes it where Spark fallback previously supplied correct results. Match Spark's accepted syntax or retain fallback for affected targets. Evidence: An exact-head disposable native test evaluated `Variant::String("NaNf")` and `Variant::String("Infinityf")` as Float32 and Float64. TRY returned non-null NaN/Infinity, and strict evaluation succeeded in every case. Direct Spark 4.1.3 and 4.2.0 probes returned NULL in TRY mode and `INVALID_VARIANT_CAST` in strict mode. Spark's `Cast.scala` first uses Java floating-point parsing and then matches complete special literals, neither of which accepts these suffixed forms. ########## native/spark-expr/src/conversion_funcs/string.rs: ########## @@ -254,7 +255,44 @@ where } else { s }; - // Rust's parse logic already handles scientific notations so we just rely on it + let unsigned = pruned_float_str + .strip_prefix(['+', '-']) + .unwrap_or(pruned_float_str); + if unsigned.starts_with("0x") || unsigned.starts_with("0X") { + // lexical 1.0.6's fast path cannot mix radix 16 with a binary exponent. Expand + // only the significand, preserving exact bits and rounding at the target width. + // Remove after https://github.com/Alexhuszagh/rust-lexical/issues/87 is fixed. + let (mantissa, exponent) = unsigned[2..].split_once(['p', 'P'])?; + let mut binary = String::new(); + if pruned_float_str.starts_with('-') { + binary.push('-'); + } + for digit in mantissa.bytes() { + if digit == b'.' { + binary.push('.'); + } else { + let digit = char::from(digit).to_digit(16)?; + for bit in (0..4).rev() { + binary.push(char::from(b'0' + ((digit >> bit) & 1) as u8)); + } + } + } + binary.push('p'); + binary.push_str(exponent); + const BINARY: u128 = NumberFormatBuilder::new() + .mantissa_radix(2) + .exponent_base(std::num::NonZeroU8::new(2)) + .exponent_radix(std::num::NonZeroU8::new(10)) + .required_exponent_notation(true) + .no_special(true) + .build_strict(); + return F::from_lexical_with_options::<BINARY>( Review Comment: [P2] Preserve rounding at the hexadecimal underflow boundary. With `spark.sql.variant.pushVariantIntoScan=false`, store `parse_json('"0x1.8p-150"')` in a Parquet Variant column `v` and select `try_variant_get(v, '$', 'float')`. Spark returns the smallest positive FLOAT, but this parser returns zero. DOUBLE extraction of `"0x1.8p-1075"` has the same problem. Strict extraction also silently returns zero. These previously correct fallback queries now produce incorrect values. Use conversion that rounds subnormals before underflowing, and add cases between zero and the minimum subnormal. Evidence: At the exact head, a disposable test passed `Variant::String("0x1.8p-150")` to `VariantGet::evaluate_array` targeting Float32, and `Variant::String("0x1.8p-1075")` targeting Float64. Both strict and TRY modes returned raw bits `0`. Direct `VariantGet.variantGet` calls on Spark 4.1.3 and 4.2.0 returned raw bits `1` in all four cases. The inputs equal 0.75 of the respective minimum subnormal and must round upward. This is a distinct rounding failure in the newly added parser. -- 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]
