xiangfu0 commented on code in PR #18115: URL: https://github.com/apache/pinot/pull/18115#discussion_r3048718082
########## 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: Fixed — changed `intBitExtract` and `longBitExtract` to accept `long bit`. The existing `isBitIndexInRange(long, int)` validates the full long value before casting, so values like 2^32+1 correctly return 0 instead of silently truncating to an in-range int. See commit 69bf8a6. ########## 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: The PR description already calls this out explicitly: > *For ingestion transforms (arity-only resolution via `getFunctionInfo(int)`), the LONG overload is returned as a safe fallback — INT inputs widen losslessly via `convertTypes`* The test comment documents this intentionally so the behavior difference is clear to future readers. -- 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]
