yashmayya commented on code in PR #19757: URL: https://github.com/apache/pinot/pull/19757#discussion_r4201344300
########## pinot-core/src/main/java/org/apache/pinot/core/function/scalar/IdSetFunctions.java: ########## @@ -0,0 +1,162 @@ +/** + * 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.core.function.scalar; + +import com.google.common.annotations.VisibleForTesting; +import java.io.IOException; +import java.math.BigDecimal; +import java.sql.Timestamp; +import java.util.UUID; +import javax.annotation.Nullable; +import org.apache.pinot.core.query.utils.idset.IdSet; +import org.apache.pinot.core.query.utils.idset.IdSets; +import org.apache.pinot.spi.annotations.ScalarFunction; +import org.apache.pinot.spi.data.FieldSpec.DataType; +import org.apache.pinot.spi.utils.ByteArray; +import org.apache.pinot.spi.utils.UuidUtils; + + +/// Scalar form of the `IN_ID_SET` transform function, for queries that cannot run the transform function: +/// - In the multi-stage engine, wherever the predicate is not pushed into a leaf stage, e.g. a filter over a join, +/// `HAVING` or a join condition. Leaf stages keep using the `InIdSetTransformFunction`, because transform functions +/// take precedence over scalar functions. +/// - In the single-stage engine, after aggregation, e.g. in `HAVING`. Aggregations such as `MAX` return DOUBLE there, +/// so cast them to the type the IdSet was built from. +/// +/// The method is not static, so each call site gets its own instance (see `FunctionInvoker`), which caches the last +/// deserialized IdSet: +/// - A literal IdSet reaches every row as the same String instance, so it is deserialized once per call site. +/// - An IdSet read from rows (e.g. a joined `IDSET` subquery result) arrives as a new but equal String instance with +/// every data block, which costs one String comparison per block. +/// - A call site whose IdSet changes from row to row (e.g. a correlated `IDSET` subquery, with one IdSet per group) +/// deserializes the IdSet on every change, so it only suits small IdSets. +/// +/// Not thread-safe: an instance serves one call site, which one thread evaluates at a time. +public class IdSetFunctions { + private String _serializedIdSet; + private IdSet _idSet; + // The IdSet and the value class whose combination passed the last value type validation + private IdSet _validatedIdSet; + private Class<?> _validatedValueClass; + + /// Returns whether the IdSet contains the value. As with an `IN` subquery, the result is `false` for an empty or + /// `null` IdSet, even when the value is `null`, and `null` for a `null` value otherwise. `IDSET` returns a `null` + /// IdSet over zero rows when null handling is enabled. + /// + /// Values arrive as their external Java types. BOOLEAN, TIMESTAMP and UUID values are looked up through their + /// stored INT, LONG and BYTES values, as the transform function does. + /// + /// `isDeterministic = false` keeps all-literal calls out of compile-time evaluation, which passes literals as Java + /// types the lookup cannot map to a value type (e.g. BigDecimal for an INT literal). The runtime evaluates them + /// instead, as before this function existed. The flag also marks the function VOLATILE, so ingestion transforms + /// cannot use it. + @Nullable + @ScalarFunction(nullableParameters = true, isDeterministic = false) Review Comment: Done in 021374bb0e. The comment is in `IdSetFunctions` and at `TransformFunctionType.IN_ID_SET`, and new cases in `JoinPlans.json` and `PhysicalOptimizerPlans.json` pin the push-down. When I made the `IN_ID_SET` operator volatile, both cases failed, with the filter above the join. I kept `isDeterministic = false` instead of mapping BigDecimal to the set's type. A Bloom set does not record whether it was built from INT or FLOAT values (or LONG or DOUBLE), so the mapping could look up the wrong bits. The root cause is that `PinotEvaluateLiteralRule` passes raw Calcite values (BigDecimal for every exact number) to `Object` parameters, where the runtime passes Integer or Long. From the code, this also changes fold-time results of other `Object`-parameter functions, such as `toThetaSketch`. So I think the fix belongs in the rule, in a separate PR. ########## pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InIdSetTransformFunction.java: ########## @@ -61,16 +63,24 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c _transformFunction = arguments.get(0); try { _idSet = IdSets.fromBase64String(((LiteralTransformFunction) arguments.get(1)).getStringLiteral()); - } catch (IOException e) { - throw new IllegalArgumentException("Caught exception while deserializing IdSet", e); + } catch (IOException | RuntimeException e) { + throw new IllegalArgumentException("Caught exception while deserializing IdSet: " + e, e); } + IdSets.validateValueType(_idSet, _transformFunction.getResultMetadata().getDataType().getStoredType()); } @Override public TransformResultMetadata getResultMetadata() { return BOOLEAN_SV_NO_DICTIONARY_METADATA; } + /// As with an `IN` subquery, a NULL value is in no empty IdSet, so the result is never NULL for an empty IdSet. + @Nullable + @Override + public RoaringBitmap getNullBitmap(ValueBlock valueBlock) { + return _idSet.getType() == IdSet.Type.EMPTY ? null : super.getNullBitmap(valueBlock); Review Comment: Done in 021374bb0e. I added the `backward-incompat` label, a release note in the description, and `IdSetQueriesTest.testInIdSetNullHandling` (single-stage, nullable column, null handling on, empty and non-empty sets). The docs line needs a separate PR in the docs repo. One note on `IN_SUBQUERY`: its subquery takes null handling from the broker config or the request options, not from the outer query's `SET`. If the subquery runs with null handling on and matches no rows, `IDSET` returns NULL, and the rewrite already fails with a `NullPointerException` today. I listed that under Known limits. -- 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]
