Copilot commented on code in PR #18115: URL: https://github.com/apache/pinot/pull/18115#discussion_r3048441679
########## pinot-common/src/main/java/org/apache/pinot/common/function/scalar/bitwise/BitExtractScalarFunction.java: ########## @@ -0,0 +1,96 @@ +/** + * 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.pinot.common.function.scalar.bitwise; + +import javax.annotation.Nullable; +import org.apache.pinot.common.function.FunctionInfo; +import org.apache.pinot.common.function.PinotScalarFunction; +import org.apache.pinot.common.function.sql.PinotSqlFunction; +import org.apache.pinot.common.utils.DataSchema.ColumnDataType; +import org.apache.pinot.spi.annotations.ScalarFunction; + + +/** + * Polymorphic bit extraction scalar function. + * + * <p>This implementation is stateless and thread-safe. + */ +@ScalarFunction(names = {"bitExtract", "extractBit"}) +public class BitExtractScalarFunction implements PinotScalarFunction { + private static final FunctionInfo INT_FUNCTION_INFO; + private static final FunctionInfo LONG_FUNCTION_INFO; + + static { + try { + INT_FUNCTION_INFO = + new FunctionInfo(BitExtractScalarFunction.class.getMethod("intBitExtract", int.class, int.class), + BitExtractScalarFunction.class, false); + LONG_FUNCTION_INFO = + new FunctionInfo(BitExtractScalarFunction.class.getMethod("longBitExtract", long.class, int.class), + BitExtractScalarFunction.class, false); + } catch (NoSuchMethodException e) { + throw new RuntimeException(e); + } + } + + @Override + public String getName() { + return "bitExtract"; + } + + @Override + public PinotSqlFunction toPinotSqlFunction() { + return BitFunctionUtils.extractSqlFunction(getName()); + } + + @Nullable + @Override + public FunctionInfo getFunctionInfo(ColumnDataType[] argumentTypes) { + if (argumentTypes.length != 2) { + return null; + } + ColumnDataType valueType = argumentTypes[0].getStoredType(); + ColumnDataType bitType = argumentTypes[1].getStoredType(); + if (!BitFunctionUtils.isIntegral(valueType) || !BitFunctionUtils.isIntegral(bitType)) { + return null; + } Review Comment: `getFunctionInfo()` allows the bit index argument to be `LONG` (`isIntegral(bitType)`), but the registered methods take `int bit`. This means BIGINT bit indexes will be silently truncated by `convertTypes()`, so values outside the INT range (or e.g. 2^32+1) can wrap into an in-range `int` and return an incorrect non-zero bit. Either (1) provide overloads that accept a `long bit` and range-check using the full long value before casting, or (2) reject `LONG` bit-index arguments at function-resolution/SQL signature level so users must pass INT. ########## pinot-segment-local/src/test/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluatorTest.java: ########## @@ -236,6 +236,24 @@ public void testNullReturnedByInbuiltFunctionEvaluatorThatCannotTakeNull() { } } + @Test + public void testPolymorphicBitwiseFunctions() { + // Ingestion evaluator resolves by arity, which returns the LONG overload. + // INT inputs are widened to LONG via convertTypes, so results are LONG. + GenericRow intRow = new GenericRow(); + intRow.putValue("value", 6); + intRow.putValue("rhs", 3); + intRow.putValue("shift", 2); + assertEquals(new InbuiltFunctionEvaluator("bitNot(value)").evaluate(intRow), -7L); + assertEquals(new InbuiltFunctionEvaluator("bitAnd(value, rhs)").evaluate(intRow), 2L); + assertEquals(new InbuiltFunctionEvaluator("bitShiftRightUnsigned(value, shift)").evaluate(intRow), 1L); Review Comment: This test (and its comment) codifies that ingestion-time evaluation resolves by arity and therefore widens INT inputs to the LONG overload, returning `Long` results. The PR description says INT-only inputs preserve INT return types and INT semantics; please either adjust the PR description to call out the ingestion-time behavior difference explicitly, or update ingestion-time resolution so INT-only inputs actually use the INT overloads. -- 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]
