Jackie-Jiang opened a new pull request, #19201: URL: https://github.com/apache/pinot/pull/19201
## Summary `DataTableBuilder.setNull()` unconditionally wrote the `OBJECT` encoding — an 8-byte `(offset, length)` pair in the fixed slot plus a `CustomObject.NULL_TYPE_VALUE` marker in the variable buffer — regardless of the column's type. Per `DataTableUtils.computeColumnOffsets`, `INT` / `FLOAT` / `STRING` slots are only 4 bytes, so that write: | Column stored type | Slot | Result | |---|---|---| | `INT` / `FLOAT` / `STRING` / `BOOLEAN`, last column | 4B | `BufferOverflowException` | | `INT` / `FLOAT` / `STRING` / `BOOLEAN`, not last | 4B | silently clobbers the next column's first 4 bytes | | `LONG` / `DOUBLE` / `TIMESTAMP` | 8B | in-bounds but decodes as `varOffset << 32` | | `BIG_DECIMAL` / `BYTES` / MV arrays | 8B | decodes as length 0, or throws on deserialize | | `OBJECT` / `UNKNOWN` / `MAP` | 8B | correct — the only intended use | The aggregate and group-by paths reach it whenever null handling is disabled. Aggregation functions whose accumulator has no identity element return a null intermediate result in **both** modes, and their result column type is not `OBJECT`: - `MinStringAggregationFunction` / `MaxStringAggregationFunction` — intermediate type `STRING` - `AnyValueAggregationFunction` — intermediate type is the input column's type - any `extractFinalResult` returning null on the `serverReturnFinalResult` path So `SELECT MAX(stringCol) FROM t WHERE <matches nothing>` with `enableNullHandling=false` throws `BufferOverflowException` on the server, and `SELECT ANY_VALUE(intCol), SUM(x) ...` silently corrupts the `SUM`. ## Fix Whether an intermediate result can be null is a property of the aggregation function, not of the query's null-handling option, so the wire format's ability to represent null should not be gated on that option. This moves null representation into the builder: - `setNull(colId)` keeps the in-band encoding for `OBJECT` / `UNKNOWN` / `MAP` (already read back as null via `getCustomObject` / `getMap`), and for every other type writes the column's null placeholder plus a bit in a lazily-allocated per-column `RoaringBitmap`. The placeholder is resolved on the **logical** type so `UUID` still gets the nil UUID rather than a zero-length `BYTES`. - `build()` appends the bitmap section **only when a null was actually recorded**, so a table without nulls is byte-for-byte identical to before. - `setNullRowIds` becomes a positional merge into the builder's bitmaps, retiring its `TODO: Revisit this`. The four serialization sites (`AggregationResultsBlock`, `GroupByResultsBlock`, and both reducers' `buildIntermediateDataTable`) lose their `if (nullHandlingEnabled) … else …` fork and keep only the null-aware body; the reducers restore nulls unconditionally. `GroupByDataTableReducer` hoists an "any bitmap present" check so the per-row restore is skipped entirely for tables without nulls, preserving current cost. This converges `DataTable` on the design `DataBlockBuilder` already uses for the multi-stage engine, which writes bitmaps unconditionally and reserves `setNull` for `OBJECT` / `UNKNOWN`. `QueryContext.requiresNullAwareKeySerialization()` no longer governs serialization — its remaining uses are the `TableResizer` ORDER BY comparator and `HavingFilterHandler` — so it is renamed `requiresNullAwareKeyEvaluation()`. `SelectionOperatorUtils` is deliberately untouched: it only calls `setNull` for `UNKNOWN` columns, so selection DataTables are unchanged on the wire. ## Overhead - Fixed section, only when a null exists: `8 × numColumns` bytes, independent of row count. - Variable section: bounded by 1 bit per row per nullable column (RoaringBitmap caps a container at 8192 bytes per 65536 rows). - No new work in any per-row or per-cell loop on the write side. The read side adds `numColumns` `getNullRowIds` calls per DataTable in disabled mode, each an add and two compares when the section is absent. - Null-handling-enabled queries with no nulls in the result get **smaller**: today `setNullRowIds` is always called and burns `8 × numColumns` bytes on all-zero entries. Worst realistic case measured analytically — a 100k-group, 5-column group-by with one 30%-null column — is ~16.4 KB on a ~3.2 MB table, about 0.5%. ## Backward compatibility The format change is additive. `_fixDataSize` is computed from the schema rather than the byte-array length, and `getNullRowIds` already returns null when the computed position is past the buffer limit, so an old reader ignores the trailing bytes. Nothing else in the tree reads `_fixedSizeDataBytes.length`, and V4 is the only builder version. One rolling-upgrade constraint: a new server can emit a bitmap in disabled mode that an old broker would ignore, reading the placeholder as a real value. **Brokers must be upgraded before servers, and rolled back after them** — i.e. broker version >= server version at all times, which makes that combination unreachable. Worth a release note. The Spark connector reads server DataTables directly over gRPC, outside the cluster upgrade order, but `PinotScanBuilder` declares only `SupportsPushDownFilters` / `SupportsPushDownRequiredColumns` and `ScanQueryGenerator` emits no aggregations, so it never receives one of the affected tables. It also already reads `getNullRowIds` unconditionally. ## Note for reviewers `DataTableSerDeTest` carries an unrelated `Assert.assertX(...)` → `import static org.testng.Assert.*` conversion across the file, which inflates its diff by ~58 lines beyond the new tests. -- 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]
