Jackie-Jiang commented on PR #19439: URL: https://github.com/apache/pinot/pull/19439#issuecomment-5522122486
Addressed in the latest push (single amended commit `2168a68`). **Critical** 1. Nullable `OpenStructDataSource` contract: not taken. The interface is in no Apache release (added by #18368, first ships in 1.6.0) and has no implementation outside this repo, so there is no third-party implementation to stay compatible with. Keeping the method nullable would keep the three call-site fallbacks this PR removes. 2. Sparse JSON fast path vs custom default null values: fixed. `trySparseJsonIndex` now refuses the key's actual default null value instead of only the STRING sentinel, so a manifest-absent key with a custom default takes the per-key fold and agrees with fully materialized segments. Covered by a new case in `testSparseJsonIndexRefusals`. 3. Real MAP absent-key path: added `BaseMapDataSourceTest`, which drives `BaseMapDataSource.getDataSource` with a `MapIndexReader` that reports the key absent and asserts the value-typed all-null source. An end-to-end query cannot reach this path in this repo: neither `MapIndexReader` implementation ever reports a key absent, and `MapFieldTypeTest` already covers absent keys through that path on both engines. **Major** - Constant folding: the previous fold also built the all-null source before evaluating, and the `IS_NULL` bitmap is a single run container, so nothing new is materialized per query. Left as is. - `ImmutableOpenStructDataSourceTest` line 159: fixed, it now asserts the STRING fallback type. - Renamed `getKeyFieldSpec` to `getValueFieldSpec`. - Trailer removed, and the root-cause rationale moved into the commit message. **Minor** - Deprecated wrappers: the removed classes are internal to `pinot-segment-local` and one of them is unreleased, so no shims. - Thread safety documented on `NullDataSource`. - `FieldSpec.DataType` matches the existing style of the touched test files. Also fixed the two CI failures this surfaced: the mocked `QueryContext` in `MapFilterOperatorOpenStructTest` now serves `getOrComputeSharedValue`, which the dictionary-based IN evaluator uses, and `DefaultNullValueVirtualColumnProviderTest` expects the entry count that `buildMetadata` now reports. -- 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]
