xiangfu0 opened a new pull request, #19647:
URL: https://github.com/apache/pinot/pull/19647
## Summary
This PR adds the contract for schema-bound aggregation without using it yet.
It is the first step of splitting #19523 into reviewable pieces.
- **`AggregateCallBinding`** (`pinot-common`) holds the immutable logical
argument types and result type of one aggregate call. It describes the original
SQL call, not the intermediate accumulator a stage may exchange.
- **Wire fields.** Both are optional and append-only:
- Thrift `Function.aggregationBinding` (field 3) →
`AggregationFunctionBinding { list<string> argumentTypes; string resultType }`.
The strings are `ColumnDataType` names.
- Protobuf `FunctionCall.aggregationBinding` (field 6) →
`AggregationFunctionBinding { repeated ColumnDataType argumentTypes;
ColumnDataType resultType }`.
- **`FunctionContext`** carries an optional binding, and
`RequestContextUtils` reads it from Thrift. The binding is execution metadata.
It is excluded from `FunctionContext` `equals`, `hashCode` and `toString`, so
expression identity, aggregation indexing and post-aggregation matching are
unaffected.
- **`AggregationFunctionTypeResolver`** (`pinot-common`) evaluates an
aggregate's registered Calcite `SqlReturnTypeInference` over schema-derived
operand types and literal options, without a parsed SQL tree. It preserves
logical `BOOLEAN` and `TIMESTAMP` types. Both engines will derive result types
through this one path.
- **`AggregationFunctionType` hooks**: `isTypeBindingRequired(...)` and
`supportsLegacyUnboundCalls()`. Both default to `false`, and no aggregate opts
in here.
## Follow-ups
1. Attach bindings in the single-stage engine (broker and direct server SQL)
and in the multi-stage engine (stage splitting and plan serde).
2. Opt aggregates in one family at a time, starting with the purely additive
two-argument `FIRST_WITH_TIME` / `LAST_WITH_TIME`.
## Compatibility
- Nothing sets the new fields yet, so request and plan bytes are unchanged
for every query.
- Both fields are optional and append-only. Older readers skip them; the
test covers this for the compact and binary protocols.
- The generated Thrift `Function.equals` and `hashCode` include the new
field. Once bindings are attached, request-level matching (expression
overrides, MV rewrite) has to compare `ExpressionContext`, not Thrift
`Function`. The single-stage follow-up handles this.
## Generated code
`Function.java` and the new `AggregationFunctionBinding.java` were generated
with **Thrift 0.21.0** (`--gen java:beans`), the same compiler as the other
checked-in request classes.
- Thrift 0.24.0 generates a 64-level recursion limit into `read()`. With it,
servers fail to decode deeply nested expressions such as `c1 + c2 + ... + c66`.
- The 0.21.0 compiler used here reproduces the existing master classes. The
only difference is temp-variable numbering in `Function.java`, which is
regenerated here anyway.
- A new test decodes a 100-level nested expression with a default
`TDeserializer`. Against the 0.24.0-generated `Function` it fails with
`Recursion depth 65 exceeds limit 64`.
## Tests
New `AggregateCallBindingTest`:
- round trip on the compact and binary protocols, with an older reader
skipping the field
- immutability
- binding excluded from expression identity
- return-type inference with logical types, literal options and array results
- deep nesting
--
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]
