gortiz commented on code in PR #19108:
URL: https://github.com/apache/pinot/pull/19108#discussion_r4063535119
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/function/NotUdf.java:
##########
@@ -40,7 +40,7 @@ public class NotUdf extends Udf.FromAnnotatedMethod {
public NotUdf()
throws NoSuchMethodException {
- super(LogicalFunctions.class.getMethod("not", boolean.class));
+ super(LogicalFunctions.class.getMethod("not", Boolean.class));
Review Comment:
**Correct fix, and it explains why the whole `UdfTest` class died at
`setUp()`.**
`LogicalFunctions.not` was widened from `not(boolean)` to `@Nullable Boolean
not(@Nullable Boolean)` in #17189 (2025-12-07) for three-valued NULL logic,
while `NotUdf` has carried the primitive lookup since #16258 (2025-07-24). So
`getMethod("not", boolean.class)` throws `NoSuchMethodException` in the
constructor.
Why that is fatal rather than local: `UdfTestFramework.fromServiceLoader`
does `ServiceLoader.load(Udf.class).stream().map(Provider::get)`, so a single
provider whose constructor throws raises `ServiceConfigurationError` and *no*
UDF loads at all — `@BeforeClass` fails and every test in the class is skipped.
I checked the other 11 reflective lookups under `query/runtime/function/`
(`Acos`, `Degrees`, `Year`, `ToDateTime`, `Adler32`, `Array*`, `Uuid*`,
`BytesToUuid`): all still match their target signatures, so `NotUdf` is the
only stale one. The fix is complete.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/function/NotUdf.java:
##########
@@ -40,7 +40,7 @@ public class NotUdf extends Udf.FromAnnotatedMethod {
public NotUdf()
throws NoSuchMethodException {
- super(LogicalFunctions.class.getMethod("not", boolean.class));
+ super(LogicalFunctions.class.getMethod("not", Boolean.class));
Review Comment:
**The CI gap is the part I would not defer** — there is now hard evidence
the snapshots have been maintained by hand for nine months.
Confirmed the claim in the description: the shard includes in
`pinot-integration-tests/pom.xml` are
`**/org/apache/pinot/integration/tests/<LETTER>*Test.java`, a single path
segment, so `tests/udf/UdfTest.java` matches neither `integration-tests-set-1`
nor `set-2`.
What that has cost, concretely: the framework has been unloadable since
#17189 (2025-12-07), yet `all-functions.yaml` was edited in #18171 (2026-04),
#18591 (2026-05), #19012 (2026-07), #19091 (2026-07) and #19223 (2026-08). None
of those could have run the generator, so the snapshot has been hand-maintained
throughout. That is the mechanism that let the `min`/`max` placeholders survive
(see the note on that hunk), and it means these snapshots have provided no
regression signal at all in that window.
```xml
<include>**/org/apache/pinot/integration/tests/udf/*Test.java</include>
```
in `set-2` closes it. If it is deferred for runtime reasons, please link the
follow-up issue rather than leaving it as prose.
Separately and minor: `NotUdf` extends `Udf.FromAnnotatedMethod` but
overrides `getMainName()` and returns `null` from `getScalarFunction()`, so the
reflected `Method` feeds only `getAllNames() -> Set.of("not")`. Its siblings
`AndUdf` and `OrUdf` are plain `Udf` stubs. The class pays the reflection-drift
cost for nothing — worth simplifying, not blocking.
##########
pinot-integration-tests/src/test/resources/udf-test-results/abs.yaml:
##########
@@ -146,7 +146,7 @@ MSE intermediate stage (with null handling):
'(arg0: big_decimal) -> big_decimal':
entries:
negative value_big_decimal:
- actualResult: "3.0"
+ actualResult: "3"
Review Comment:
**This is the one snapshot delta in the PR I could not clear** — it looks
like a real execution-result change, not a stale fixture.
Scope: only the two *MSE intermediate stage* sections moved. The
ingestion-transformer and SSE-projection sections still record `"3.0"`, so
`abs` itself is innocent — `AbsScalarFunction.bigDecimalAbs` is `value.abs()`,
which preserves scale (`new BigDecimal("-3.0").abs().toString()` is `"3.0"`).
What differs is the plan. `IntermediateUdfTestScenario` wraps the argument
in `coalesce(t1.col, t2.col)` across a self-join so the call lands after the
exchange. Something on that path — Calcite DECIMAL type inference on
`COALESCE`, or the MSE BigDecimal ser/de — now drops the scale. That is
user-visible: `CAST(abs(x) AS STRING)` yields `"3"` or `"3.0"` depending only
on whether the expression was pushed to the leaf.
Why it slipped through: the equivalence is `BIG_DECIMAL_AS_DOUBLE`, so the
assertion passes either way. And the window is wide — the last regeneration
that could have been real is #17140 (2025-11-04), before #17189 broke the
framework, so the change landed somewhere in the last ten months.
`UdfTest`'s own javadoc says a changed implementation must be called out as
a behaviour change in the PR and commit message. Could you identify the commit
responsible and confirm whether losing the scale is intended?
##########
pinot-integration-tests/src/test/resources/udf-test-results/abs.yaml:
##########
@@ -267,22 +267,22 @@ MSE intermediate stage (without null handling):
'(arg0: big_decimal) -> big_decimal':
entries:
negative value_big_decimal:
- actualResult: "3.0"
+ actualResult: "3"
Review Comment:
**The companion `.md` snapshot was not committed** — `abs.md` still says
`3.0`.
`generateSnapshots()` writes `<udf>.yaml` **and** `<udf>.md` in the same
loop, so a real regeneration also rewrites `abs.md`. On this branch
`abs.md:102-104` and `129-132` still read `3.0 (String)` for exactly the rows
changed here. So either the `.md` output was left out of the commit, or the
YAML was hand-edited from the test failure message. Please re-run the generator
and `git add` the `.md` files so the two stay consistent.
Separate and pre-existing: `getSnapshotFiles()` only deletes `*.yaml` before
regenerating, so orphan `.md` files accumulate (`asin.md`, `ceil.md`, `chr.md`
… have no matching `.yaml`, and `failWhenSnapshotNotUsed` only inspects
`.yaml`). Worth a cleanup, not in this PR.
##########
pinot-integration-tests/src/test/resources/udf-test-results/abs.yaml:
##########
@@ -267,22 +267,22 @@ MSE intermediate stage (without null handling):
'(arg0: big_decimal) -> big_decimal':
entries:
negative value_big_decimal:
- actualResult: "3.0"
+ actualResult: "3"
equivalence: "BIG_DECIMAL_AS_DOUBLE"
error: null
expectedResult: 3.0
null input_big_decimal:
- actualResult: "0.0"
+ actualResult: "0"
Review Comment:
Same scale loss on the null-default path: with null handling disabled the
NULL input materialises as the BIG_DECIMAL default and now reports `"0"`
instead of `"0.0"`.
Same root cause as the negative/positive rows; flagged separately only
because the default-value path is the one most likely to surface in a user's
`CAST(... AS STRING)` output.
##########
pinot-integration-tests/src/test/resources/udf-test-results/all-functions.yaml:
##########
@@ -34,6 +34,10 @@ acos:
scalar:
"ArgumentCountBasedScalarFunction{org.apache.pinot.common.function.scalar.TrigonometricFunctions.acos}"
transform:
"org.apache.pinot.core.operator.transform.function.TrigonometricTransformFunctions.AcosTransformFunction"
udf: "org.apache.pinot.query.runtime.function.AcosUdf"
+acosh:
+ scalar:
"ArgumentCountBasedScalarFunction{org.apache.pinot.common.function.scalar.TrigonometricFunctions.acosh}"
Review Comment:
**The branch is ~2 months behind master, so this regenerated file is already
stale again by 17 functions.**
The merge base is `a8b207e781` (2026-07-27). I diffed this file against the
live registry on current master (probe below):
```
in this file, absent from the registry: 0 <- nothing invented
value mismatches: 0 <- every value correct
registered but MISSING from this file: 17
bytestouuid, isuuid, touuid, uuidtostring, uuidtobytes,
uuidtimestamp, uuidv4, uuidv7, uuidversion,
jsonextractobject, jsonextractscalarfory, jsonpathdoublefory,
jsonpathlongfory, jsonpathstringfory,
isprivateip, overlay, translate
```
The first two lines are the good news — everything this PR wrote is faithful
to the registry. But those 17 landed after the fork point (#19091 UUID, #19223
Fory and friends), and master's copy of `all-functions.yaml` already lists
them, so this needs regenerating after a rebase or
`failWhenAllFunctionsYamlNotUpdated` will fail on merge.
##########
pinot-integration-tests/src/test/resources/udf-test-results/all-functions.yaml:
##########
@@ -1149,10 +1395,6 @@ mapvalue:
scalar: null
transform:
"org.apache.pinot.core.operator.transform.function.MapValueTransformFunction"
udf: null
-max:
Review Comment:
**Checked this one and it is correct** — `min`/`max` were hand-written
placeholders that the generator has never produced.
I ran the real registry code against `pinot-core`'s test classpath (same
shape as the integration-test env, test-classes included):
```
min scalarKey=false transformKey=false
max scalarKey=false transformKey=false
scalar null-valued keys: [] transform null-valued keys: []
(534 scalar, 134 transform)
```
And an all-null entry is structurally impossible for this generator, at any
commit:
* `FUNCTION_MAP = Map.copyOf(functionMap)` — `Map.copyOf` throws NPE on a
null value, and it has been `Map.copyOf` since #16258.
* `TRANSFORM_FUNCTION_MAP` only ever receives class literals in
`createRegistry()` and a non-null class in `init()`.
* Keys come from `Sets.union(scalarKeys, transformKeys)`, so every key
resolves to a non-null value in at least one map, and `getScalarFunctionId()` /
`getCanonicalName()` on a top-level class never return null.
`min`/`max` are `AggregationFunctionType` entries, not scalar or transform
functions, and no source declares them at either #16258 or #19223. Read with
the file's header — *"a list of UDFs we should create"* — they were typed in by
hand as to-do markers and the generator has been dropping them ever since.
No action needed; one line in the description would save the next reader the
dig.
##########
pinot-integration-tests/src/test/resources/udf-test-results/all-functions.yaml:
##########
@@ -1476,7 +1747,7 @@ sqrt:
transform:
"org.apache.pinot.core.operator.transform.function.SingleParamMathTransformFunction.SqrtTransformFunction"
udf: null
stageid:
- scalar:
"ArgumentCountBasedScalarFunction{org.apache.pinot.query.function.InternalMseFunctions.stageId}"
+ scalar:
"ArgumentCountBasedScalarFunction{org.apache.pinot.common.function.scalar.InternalFunctions.stageId}"
Review Comment:
Verified this move is real, not a generation artifact:
`org.apache.pinot.query.function.InternalMseFunctions` no longer exists
anywhere in the tree, and `stageId`/`workerId` now live in
`pinot-common/.../scalar/InternalFunctions` (lines 108 and 122). Same for
`workerid` further down.
No action — noting it so the delta does not have to be re-investigated at
merge time.
##########
pinot-integration-tests/src/test/resources/udf-test-results/all-functions.yaml:
##########
@@ -51,9 +55,13 @@ agomv:
transform: null
udf: null
and:
- scalar: null
+ scalar:
"ArgumentCountBasedScalarFunction{org.apache.pinot.common.function.scalar.LogicalFunctions.and}"
transform:
"org.apache.pinot.core.operator.transform.function.AndOperatorTransformFunction"
udf: "org.apache.pinot.query.runtime.function.AndUdf"
+appendtostringandreturn:
Review Comment:
**Test-only `@ScalarFunction` fixtures are being baked into the committed
snapshot.**
`MutableStringTestFunction` lives in
`pinot-common/src/test/java/org/apache/pinot/common/function/test/`. Same story
at `counttruebooleans` and `sumtimestampmillis`, both from
`pinot-core/src/test/.../ScalarTransformFunctionWrapperTest`. My local probe
registered all three as well, so this is deterministic rather than a one-off:
`pinot-integration-tests` depends on the `pinot-common` and `pinot-core`
test-jars, and `FunctionRegistry`'s classpath scan picks them up.
Two consequences:
1. The file's stated purpose in its own header — *"a list of UDFs we should
create"* — is now polluted with test fixtures nobody should write a UDF for.
2. Once `UdfTest` runs in CI, `failWhenAllFunctionsYamlNotUpdated` will fail
whenever someone adds an unrelated test-only `@ScalarFunction` anywhere in
`pinot-core` or `pinot-common` tests, which is a baffling failure to land on.
Not introduced by this PR — it falls out of any honest regeneration — but
better filtered at generation time (skip classes whose package is a test
package) than frozen in.
##########
pinot-integration-tests/src/test/resources/udf-test-results/all-functions.yaml:
##########
@@ -51,9 +55,13 @@ agomv:
transform: null
udf: null
and:
- scalar: null
+ scalar:
"ArgumentCountBasedScalarFunction{org.apache.pinot.common.function.scalar.LogicalFunctions.and}"
Review Comment:
Useful cross-check: `and`/`or` going from `scalar: null` to
`LogicalFunctions.and` / `LogicalFunctions.or` is the same #17189 that changed
`not(boolean)` to `not(Boolean)` and broke `NotUdf`. The snapshot is finally
catching up with that PR, which is good evidence the regeneration is faithful
rather than arbitrary.
Worth a line in the description, since it is a registry-visible change
rather than noise.
--
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]
