Jackie-Jiang commented on code in PR #19127:
URL: https://github.com/apache/pinot/pull/19127#discussion_r3684363353
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/format/JsonPayloadParser.java:
##########
@@ -51,4 +54,16 @@ public interface JsonPayloadParser {
/// @throws Exception if the region is not valid for this format
Map<String, Object> parse(byte[] payload, int offset, int length)
throws Exception;
+
+ /// Parses the payload directly into {@code destination}, avoiding a
top-level per-record map when the
+ /// implementation supports it.
+ ///
+ /// @param fields fields to populate, or {@code null} to populate every
top-level field
+ /// @return {@code true} when the payload was decoded into {@code
destination}; {@code false} when the caller
+ /// should fall back to [#parse]
+ default boolean parseTo(byte[] payload, int offset, int length, @Nullable
Set<String> fields,
Review Comment:
(nit) just call it `parse` and put nullable `fields` as the last argument
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/format/JsonPayloadParser.java:
##########
@@ -51,4 +54,16 @@ public interface JsonPayloadParser {
/// @throws Exception if the region is not valid for this format
Map<String, Object> parse(byte[] payload, int offset, int length)
throws Exception;
+
+ /// Parses the payload directly into {@code destination}, avoiding a
top-level per-record map when the
Review Comment:
(minor) Don't use `{@code destination}`, do \`destination\` instead
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/JSONMessageDecoder.java:
##########
@@ -72,6 +74,9 @@ public void init(Map<String, String> props, Set<String>
fieldsToRead, String top
}
_jsonRecordExtractor =
PluginManager.get().createInstance(recordExtractorClass);
_jsonRecordExtractor.init(fieldsToRead, null);
+ _fieldsToRead = fieldsToRead == null || fieldsToRead.isEmpty() ? null :
Set.copyOf(fieldsToRead);
Review Comment:
(minor) Use `CollectionUtils.isNotEmpty()`
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/JSONMessageDecoder.java:
##########
@@ -72,6 +74,9 @@ public void init(Map<String, String> props, Set<String>
fieldsToRead, String top
}
_jsonRecordExtractor =
PluginManager.get().createInstance(recordExtractorClass);
_jsonRecordExtractor.init(fieldsToRead, null);
+ _fieldsToRead = fieldsToRead == null || fieldsToRead.isEmpty() ? null :
Set.copyOf(fieldsToRead);
+ // A configured extractor can change conversion semantics, so only bypass
Pinot's exact default class.
+ _usesDefaultRecordExtractor = _jsonRecordExtractor.getClass() ==
JSONRecordExtractor.class;
Review Comment:
Why do we need this check?
##########
pinot-plugins/pinot-input-format/pinot-json/src/main/java/org/apache/pinot/plugin/inputformat/json/format/JacksonPayloadParser.java:
##########
@@ -31,15 +37,71 @@
/// The [ObjectReader] is immutable and thread-safe, so a single instance is
shared across all decode calls.
abstract class JacksonPayloadParser implements JsonPayloadParser {
+ private final ObjectMapper _mapper;
Review Comment:
Not introduced in this PR, but should we use `JsonUtils` for the parsing? We
should already have all utils, and proper handling of big decimal
--
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]
