anshul98ks123 commented on PR #18317: URL: https://github.com/apache/pinot/pull/18317#issuecomment-4406287947
> The core issue is that we cannot differentiate default value vs user explicit setting. Ideally we should be able to tell which one is explicitly configured, so that when default behavior changes, explicit config won't be swallowed. We should change all primitive types to Object wrapper and use `null` to represent not defined config. This way we can simply put a json annotation to ignore null when serializing the values Thanks. Done in c5c5b688ef. Every primitive `*IndexConfig` field is now a wrapper (`Boolean`/`Integer`/`Double`); the `@JsonCreator` constructor stores the input verbatim so `null` carries the "not configured" signal, and the curated `@JsonValue toJsonObject()` emits a key only when the field is non-null. Explicit user settings that happen to match today's static default (e.g. `luceneUseCompoundFile=true`, `version=2`) now survive the round-trip and won't be swallowed if the default later flips. Public getters keep their primitive return type and apply the static default at read time, so consumer call sites are unchanged. Validation guards (`Preconditions.checkArgument` for `fpp` range, intern-requires-onHeap, etc.) are preserved and gated on non-null inputs. Two carve-outs worth flagging: - `IndexConfig.disabled` normalizes `Boolean.FALSE → null` in the constructor — every config defaults to enabled and that universal default is extremely unlikely to flip; emitting `"disabled":false` everywhere is pure ZK noise. Happy to drop the normalization if you want strict explicit-`false` preservation here too. - `ForwardIndexConfig` cluster-tunable trio uses the snapshot model from the original PR (constructor coerces `null → live JVM default` at init) so the trio is always materialized. The "explicit-vs-not-configured" distinction is intentionally lost here only — the trio's defaults are operator-controlled, so the distinction has no business meaning. Pinned by `ForwardIndexConfigSerializationTest.testTrioSnapshotsAtConstruction`. Each `*SerializationTest` now exercises the full round-trip matrix (`testRoundTripAllNull` / `testRoundTripPartial` / `testRoundTripAllSet`) plus a `testExplicit*Preserved` assertion that pins the load-bearing semantic. -- 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]
