gortiz commented on PR #18317:
URL: https://github.com/apache/pinot/pull/18317#issuecomment-4420686944
The implementation is thorough and the tests are unusually strong. Three
concerns and one design question.
**Design question — `@JsonValue toJsonObject()` vs `@JsonInclude(NON_NULL)`**
Since the fields are already wrapper types after this PR, the null-as-absent
data model is in place. The standard Jackson approach would be
`@JsonInclude(JsonInclude.Include.NON_NULL)` on the class — no manual
`toJsonObject()` needed. The reason it doesn't work out of the box is that
Jackson defaults to serializing through public getters, which return primitives
(never null). But this is solvable:
```java
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.ANY,
getterVisibility = JsonAutoDetect.Visibility.NONE)
public class BloomFilterConfig extends IndexConfig { ... }
```
The maintenance advantage: new fields added in the future are handled
automatically, while `toJsonObject()` must be updated manually (easy to forget
a field silently). Is there a specific reason `toJsonObject()` was preferred?
**Concern — `TextIndexConfig.AbstractBuilder` copy-constructor erases
null-vs-explicit**
`new TextIndexConfigBuilder(existingConfig).build()` reads through the
default-applying primitive getters into primitive builder fields. An
explicit-default value (e.g. `luceneUseCompoundFile=true`, which is today's
default) is read as `true`, compared against the static default, and becomes
`null` in the output. The explicit setting is silently dropped. The PR
documents this in Javadoc, but there is no test that exercises this path —
either to pin the behavior as intended or to document it as a known gap.
**Concern — `JsonIndexConfig.equals()` now covers `_indexPaths` and
`_maxBytesSize`**
These fields were missing from the old `equals()`. This is a correctness
fix, but it is a silent behavioral change for any code that compared
`JsonIndexConfig` instances and relied on those fields being ignored. Worth a
note in the description that this is an intentional side-fix, not just a
structural change.
**Concern — `DictionaryIndexConfig.DEFAULT` semantic change**
`DEFAULT` changed from `new DictionaryIndexConfig(false)` to `new
DictionaryIndexConfig((Boolean) null, ...)`. Any code comparing a config
deserialized from fat JSON `{"onHeap":false}` against `DEFAULT` using
`equals()` will now return `false` where it previously returned `true`.
Production callers checked don't rely on this equality, but it's worth a note
since it's a quiet `equals()` contract change.
--
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]
