gortiz commented on PR #17628: URL: https://github.com/apache/pinot/pull/17628#issuecomment-3889557968
It is common in old Java POJOs to serialize complex data structures as Strings. I assume it was done that way because Jackson lacked knowledge or because we were in a rush. But that is not a good way to solve the problem. Jackson is perfectly capable of handling complex, dynamic structures. When IndexType was designed, one of the objectives was to support complex concepts as JSON nodes and these properties that are problematic here were added afterwards (in https://github.com/apache/pinot/pull/13003), breaking the logic we already established (ie _stopWordsInclude was already serialized as a complex type). The code in this PR solves that issue. The correct serialized format should be a JSON list, not a JSON String. Given that we need to support both formats in order not break compatibility, this PR relaxes the constructor to use an Object, which may not be great. I agree that using a custom deserializer is better, but I don't follow the logic. If we keep the old constructor, third-party JSON libraries like json4s will still fail to deserialize the official format that uses a list. @anshul98ks123 can you create a PR where: 1. Recover the old constructor that accepts strings, but mark it as deprecated. 2. Adds a new constructor that accepts a List instead of a String for these two properties 3. Create and register a custom deserializer that supports both String and List for these two fields. It should always call the constructor that accepts the List. -- 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]
