gortiz commented on code in PR #19757: URL: https://github.com/apache/pinot/pull/19757#discussion_r4193532955
########## 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: `isDeterministic = false` works around literal typing (BigDecimal for an INT literal), but it also marks the function volatile. That is harmless for planning today only because `PinotOperatorTable` registers `TransformFunctionType.IN_ID_SET` before the scalar functions, so Calcite sees the transform's operator, which is deterministic. If that operator ever goes away, the planner picks this scalar's `PinotSqlFunction` instead. Then `PinotRuleUtils.isRelocatable()` returns false and `PinotFilterJoinRule` stops pushing `IN_ID_SET` below joins. Results stay correct, but the leaf transform path is lost silently. Could we add a comment here that names this coupling, and a plan test (EXPLAIN) that pins a filter over a join down to the leaf? Another option is to map BigDecimal literals to the set's type in `getStoredType` and keep the function deterministic. ########## 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: This changes existing results. With null handling on, `IN_ID_SET(nullableCol, <empty set>)` used to be NULL for NULL rows and is now FALSE, so `= 0` or `NOT IN_ID_SET(...)` now keeps those rows. The single-stage `IN_SUBQUERY` changes too when the subquery matches no rows. The PR description explains it, but I think it needs the backward-incompat label and a release note, plus a line in the `IN_ID_SET` / `IN_SUBQUERY` docs. Also, only the multi-stage leaf case in `IdSet.json` covers this today. A single-stage case in `IdSetQueriesTest` (empty set, nullable column, null handling on) would lock it in for that engine as well. -- 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]
