Copilot commented on code in PR #12873: URL: https://github.com/apache/gluten/pull/12873#discussion_r4074141611
########## cpp/velox/operators/functions/overlay/Conv.h: ########## @@ -0,0 +1,102 @@ +/* + * 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. + */ + +#pragma once + +#include <folly/Likely.h> + +#include <charconv> +#include <cstdint> +#include <system_error> +#include <vector> + +#include "velox/common/base/Exceptions.h" +#include "velox/functions/sparksql/SparkQueryConfig.h" +#include "velox/functions/sparksql/String.h" + +namespace gluten { + +/// conv(num, fromBase, toBase) -> varchar +/// +/// Overrides Velox's conv, which always lets the conversion overflow. Spark +/// only does that with ANSI mode off; with ANSI mode on, an input whose digits +/// do not fit in an unsigned 64-bit integer raises an error instead of being +/// saturated. Everything else, including the conversion itself, is delegated to +/// Velox's implementation. +template <typename T> +struct ConvFunction { + VELOX_DEFINE_FUNCTION_TYPES(T); + + // ASCII input always produces ASCII result. + static constexpr bool is_default_ascii_behavior = true; + + void initialize( + const std::vector<facebook::velox::TypePtr>& /*inputTypes*/, + const facebook::velox::core::QueryConfig& config, + const arg_type<facebook::velox::Varchar>* /*num*/, + const int32_t* /*fromBase*/, + const int32_t* /*toBase*/) { + ansiEnabled_ = facebook::velox::functions::sparksql::SparkQueryConfig{config}.ansiEnabled(); + } + + bool call( + out_type<facebook::velox::Varchar>& result, + const arg_type<facebook::velox::Varchar>& num, + int32_t fromBase, + int32_t toBase) { + if (FOLLY_UNLIKELY(ansiEnabled_) && overflows(num, fromBase, toBase)) { + // Same wording as Spark's QueryExecutionErrors.overflowInConvError. The + // simple function framework turns this into a per-row error. + VELOX_USER_FAIL("Overflow in function conv()"); + } + return delegate_.call(result, num, fromBase, toBase); + } + + private: + using VeloxConvFunction = facebook::velox::functions::sparksql::ConvFunction<T>; + + /// Returns true when the digits of 'num' do not fit in an unsigned 64-bit + /// integer. That is exactly when Spark's NumberConverter.encode() reports an + /// overflow: its two checks together detect that accumulating the next digit + /// would pass 2^64 - 1. Locates and parses the digits the same way Velox's + /// conv does, so the two agree on where the digits end. + static bool overflows(const arg_type<facebook::velox::Varchar>& num, int32_t fromBase, int32_t toBase) { + if (!VeloxConvFunction::checkInput(num, fromBase, toBase)) { + // An empty input or an out-of-range base gives NULL, in ANSI mode too. + return false; + } + auto position = static_cast<size_t>(VeloxConvFunction::skipLeadingSpaces(num)); + if (position == num.size()) { + // All spaces. + return false; + } + // Skips the negative symbol, std::from_chars does not accept one for an + // unsigned type. Spark applies the sign after the digits are accumulated, + // so it does not affect whether the digits overflow. + if (num.data()[position] == '-') { + ++position; + } + uint64_t value; + const auto status = std::from_chars(num.data() + position, num.data() + num.size(), value, fromBase); Review Comment: `std::from_chars` requires the base to be in [2, 36]. Spark/Velox `conv` commonly treats negative bases as valid (sign is a separate semantic), and `checkInput` may accept them. Passing a negative `fromBase` into `from_chars` will not reliably detect overflow (and may yield `invalid_argument`), causing ANSI overflow errors to be missed. Use the absolute value of `fromBase` for overflow detection (the sign of the base doesn’t change whether the digit sequence overflows). ########## backends-velox/src/main/scala/org/apache/gluten/expression/ExpressionRestrictions.scala: ########## @@ -83,6 +83,44 @@ object Unbase64Restrictions extends ExpressionRestrictions { override val restrictionMessages: Array[String] = Array(NOT_SUPPORT_FAIL_ON_ERROR) } +object EltRestrictions extends ExpressionRestrictions { + val NOT_SUPPORT_FAIL_ON_ERROR_MISMATCH: String = + s"${ExpressionNames.ELT} whose failOnError disagrees with the session's " + + s"'${SQLConf.ANSI_ENABLED.key}' is not supported, since Velox derives the ANSI " + + s"behavior of elt from the session config" + + override val functionName: String = ExpressionNames.ELT + + override val restrictionMessages: Array[String] = Array(NOT_SUPPORT_FAIL_ON_ERROR_MISMATCH) +} + +object ConvRestrictions extends ExpressionRestrictions { + val NOT_SUPPORT_ANSI_ENABLED_MISMATCH: String = + s"${ExpressionNames.CONV} whose ansiEnabled disagrees with the session's " + + s"'${SQLConf.ANSI_ENABLED.key}' is not supported, since Velox derives the ANSI " + + s"behavior of conv from the session config" + + override val functionName: String = ExpressionNames.CONV + + override val restrictionMessages: Array[String] = Array(NOT_SUPPORT_ANSI_ENABLED_MISMATCH) +} + +object ElementAtRestrictions extends ExpressionRestrictions { + val NOT_SUPPORT_FAIL_ON_ERROR_MISMATCH: String = + s"${ExpressionNames.ELEMENT_AT} over an array whose failOnError disagrees with the " + + s"session's '${SQLConf.ANSI_ENABLED.key}' is not supported, since Velox derives the " + + s"ANSI behavior of element_at from the session config" + + val NOT_SUPPORT_DEFAULT_VALUE_OUT_OF_BOUND: String = + s"${ExpressionNames.ELEMENT_AT} with a default value for an out-of-bound index is not " + + s"supported in Velox, which always returns NULL for such an index" Review Comment: This restriction message is now inaccurate given the overlay implementation: for arrays, Velox behavior depends on ANSI mode (NULL with ANSI off, error with ANSI on), and the message states it 'always returns NULL'. Please reword to focus on the actual unsupported feature (the default value parameter), without asserting a single out-of-bounds behavior. -- 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]
