Jackie-Jiang commented on PR #19072: URL: https://github.com/apache/pinot/pull/19072#issuecomment-5075835967
Pushed an update that substantially expands test coverage (the earlier patch-coverage gap was mostly the per-type comparison branches in `NullValueVectorHandler`) and addresses two issues found while reviewing: **Coverage added** - **Every data type**, single- and multi-value: all 11 dimension types (`INT`, `LONG`, `FLOAT`, `DOUBLE`, `BIG_DECIMAL`, `BOOLEAN`, `TIMESTAMP`, `STRING`, `JSON`, `BYTES`, `UUID`) plus all 6 metric types, whose default null values differ (`0`/`0.0` instead of `MIN_VALUE`/`-Infinity`). This covers the logical types that map onto another stored type (`BOOLEAN`→`INT`, `TIMESTAMP`→`LONG`, `JSON`→`STRING`, `UUID`→`BYTES`) and their round-trip through the segment metadata's `DEFAULT_NULL_VALUE`. - **Schema-configured (non-default) default null values**, which resolve via a different path (`DataType.convert`) than the built-in constants. Includes a `BIG_DECIMAL` case whose configured scale (`-1.50`) differs from the stored scale (`-1.5`, trailing zeros stripped at ingestion) and therefore only matches because the comparison uses `compareTo` rather than `equals`. - **The creation path** for the new `nonNull` column-metadata flag, **idempotency** across reloads, a column **missing from the segment**, and **config serde** (both the config itself and a full `TableConfig` round-trip). **Fixes from this round** - The time column is the one column whose stored null value is not always its default: ingestion substitutes the current time when the configured default falls outside the valid time range, while the metadata records the field spec's default. A backfill scan would never match, and would then record the column as containing no nulls. `TableConfigUtils` now rejects a backfill opt-in on the time column in exactly that case (an explicit in-range default is stored as-is and remains supported), sharing the range check with ingestion so the two cannot drift. Covered end to end for both branches. - `NullValueVectorHandler` now reuses `NullValueVectorCreator` rather than serializing its own bitmap, so backfilled and freshly created vectors are written by the same code path — including the run-length encoding that `RoaringBitmapWriter` applies by default. No change to the serialized format. -- 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]
