tarun11Mavani commented on PR #19154: URL: https://github.com/apache/pinot/pull/19154#issuecomment-5196377574
> Nice, careful PR — the absent-key three-valued-logic reasoning and the JSON-index refusal rules are well thought through, and test coverage is broad. Regression risk looks low: the new read paths are all gated on a sparse column existing, so fully-materialized OPEN_STRUCT and non-OPEN_STRUCT tables are untouched, and the new `reserveNextDictId` guard can't fire in its only caller (init, empty index). > > A few correctness/perf items inline. Two are potential wrong-results paths I couldn't rule out from the diff (the JSON-index equivalence for non-scalar values, and dense/sparse coercion parity) — those are the ones I'd want resolved before approving. > > Question on testing & confidence: how confident are we in dense↔sparse parity beyond the happy path? `OpenStructSparseDenseParityTest` looks like it covers well-typed values — does it also exercise (a) type-mismatched values (e.g. a non-numeric string under a declared INT key), (b) non-scalar values (object/array) under a STRING key, and (c) the opt-in JSON-index path vs. the scan path for the same dataset? If those three are covered and agree, most of my concern here goes away. Also — has this been run end-to-end on a realtime→sealed segment with a >10k-doc block to confirm the parse-cache behavior under eviction? Thank you foe the review. Replied to inline comments. regarding the testing, (a) type mismatched values are never indexed so it's not an issue. (b) any non-scaler value is stored as a string. added a test to pin this. (c) Will follow up with a pr for this improvement on json fast path. I had a pretty large test where a key was dense (int) in one segment and sparse (string) in another segment and validated that broker will cast the int to bigger datatype (string) in that case. Avoided adding the test since I am planning a PR with test files anyway and I will include it in that PR to avoid adding more LOCs in this one. -- 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]
