Copilot commented on code in PR #12873: URL: https://github.com/apache/gluten/pull/12873#discussion_r4316491417
########## 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); + } Review Comment: In ANSI mode this does a full parse in `overflows(...)` and then delegates to Velox which will parse again, doubling per-row work for `conv` when `ansiEnabled_` is true. Consider adding a cheap early-exit heuristic (e.g., based on trimmed digit length vs max digits for base) before calling `std::from_chars`, or (if acceptable) moving overflow detection into a single-pass accumulation that can also produce the converted value to avoid double parsing. ########## cpp/velox/operators/functions/overlay/ElementAt.cc: ########## @@ -0,0 +1,71 @@ +/* + * 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. + */ +#include "operators/functions/overlay/ElementAt.h" + +#include "velox/functions/lib/SubscriptUtil.h" +#include "velox/functions/sparksql/SparkQueryConfig.h" + +using namespace facebook::velox; + +namespace gluten { +namespace { + +/// Spark's element_at over an array is 1-based, negative indices count from the +/// end of the array, and an index of 0 is an error. Those match Velox's +/// element_at; 'allowOutOfBound' is the one thing ANSI mode changes, an index +/// past the end of the array giving NULL rather than an error. +/// +/// The map side of SubscriptImpl does not look at 'allowOutOfBound', so a key +/// that the map does not contain keeps giving NULL, which is what Spark does in +/// ANSI mode as well. +template <bool allowOutOfBound> +using ElementAtFunction = functions::SubscriptImpl< + /*allowNegativeIndices=*/true, + /*nullOnNegativeIndices=*/false, + allowOutOfBound, + /*indexStartsAtOne=*/true>; + +} // namespace + +std::vector<exec::FunctionSignaturePtr> elementAtSignatures() { + // The signatures do not depend on the out-of-bound behavior. + return ElementAtFunction</*allowOutOfBound=*/true>::signatures(); +} + +std::shared_ptr<exec::VectorFunction> makeElementAt( + const std::string& /*name*/, + const std::vector<exec::VectorFunctionArg>& inputArgs, + const core::QueryConfig& config) { + VELOX_CHECK_EQ(inputArgs.size(), 2); + if (!inputArgs[0].type->isArray()) { + // Same as Velox's element_at over a map, which may cache a materialized + // version of the map when it is reused across batches. + return std::make_shared<ElementAtFunction</*allowOutOfBound=*/true>>(config.isExpressionEvaluationCacheEnabled()); Review Comment: For the map case, a new `ElementAtFunction` instance is created per `makeElementAt` call, even though the only varying input here is `config.isExpressionEvaluationCacheEnabled()`. Consider caching two shared instances (caching enabled/disabled) similarly to the array case to reduce allocation overhead and keep behavior explicit. ########## docs/developers/velox-function-development-guide.md: ########## @@ -54,6 +54,15 @@ Use the overlay when: * A Velox `sparksql` function has a semantic gap with Spark. Put the corrected implementation in the overlay to override it while the fix is pending upstream. The `round` function (`overlay/Round.h`) is an example of such an override. +An overlay function follows Spark's ANSI rule the same way Velox's `sparksql` functions do: read +`SparkQueryConfig::ansiEnabled()` from the query config, which Gluten populates from the session's +`spark.sql.ansi.enabled`, then return NULL on invalid input with ANSI mode off and raise a user error with it on. When +only the ANSI behavior is missing from an otherwise correct Velox function, keep delegating to it instead of forking it: +`conv` (`overlay/Conv.h`) wraps Velox's `conv` and only adds the overflow error that ANSI mode requires, and `element_at` +(`overlay/ElementAt.h`) just picks the Velox `SubscriptImpl` instantiation that raises an error for an out-of-bound array +index. Note that Gluten still falls back on ANSI mode as a whole unless `spark.gluten.sql.ansiFallback.enabled` is set to +`false`. Review Comment: The statement “return NULL on invalid input with ANSI mode off and raise a user error with it on” is too broad and appears to conflict with behaviors exercised elsewhere in this PR (e.g., `conv` invalid bases returning NULL even with ANSI on; `element_at` map missing keys returning NULL regardless of ANSI). Please rephrase to clarify that ANSI mode affects *specific failure modes* (e.g., overflow/out-of-bound) rather than all “invalid input”, and explicitly call out exceptions like `conv` invalid bases and `element_at` map missing keys staying NULL. -- 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]
