walterddr commented on code in PR #10214:
URL: https://github.com/apache/pinot/pull/10214#discussion_r1106659291
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -181,11 +184,57 @@ private BrokerResponseNative handleRequest(long
requestId, String query,
sqlNodeAndOptions.getParseTimeNs() + (executionEndTimeNs -
compilationStartTimeNs));
brokerResponse.setTimeUsedMs(totalTimeMs);
brokerResponse.setResultTable(queryResults);
+
+ attachMetadataToResponse(stats, brokerResponse);
+
requestContext.setQueryProcessingTime(totalTimeMs);
augmentStatistics(requestContext, brokerResponse);
return brokerResponse;
}
+ private void attachMetadataToResponse(Map<String, String> stats,
BrokerResponseNative brokerResponse) {
Review Comment:
assume this is copied from QueryScheduler? it is possible to add a
comment/todo to indicate we want to consolidate later?
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -94,13 +104,26 @@ public String getType() {
public void setType(String type) {
_type = type;
}
+
+ public Map<String, String> getStats() {
+ return _stats;
+ }
+
+ public void setStats(Map<String, String> stats) {
+ _stats = stats;
+ }
}
private final Contents _contents;
public MetadataBlock(MetadataBlockType type) {
- super(0, null, new String[0], new byte[]{0}, toContents(new
Contents(type.name())));
- _contents = new Contents(type.name());
+ super(0, null, new String[0], new byte[]{0}, toContents(new
Contents(type.name(), new HashMap<>())));
+ _contents = new Contents(type.name(), new HashMap<>());
Review Comment:
```suggestion
this(type, new HashMap<>());
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -50,6 +64,17 @@ public TransferableBlock nextBlock() {
// TODO: move this to centralized reporting in broker
if (nextBlock.isEndOfStreamBlock()) {
LOGGER.info("Recorded operator stats: " + _operatorStats);
Review Comment:
logger here can be removed.
##########
pinot-common/src/test/java/org/apache/pinot/common/datablock/MetadataBlockTest.java:
##########
@@ -37,7 +38,7 @@ public void shouldEncodeContentsAsJSON()
MetadataBlock metadataBlock = new MetadataBlock(type);
// Then:
- byte[] expected = new ObjectMapper().writeValueAsBytes(new
MetadataBlock.Contents("EOS"));
+ byte[] expected = new ObjectMapper().writeValueAsBytes(new
MetadataBlock.Contents("EOS", new HashMap<>()));
Review Comment:
ditto. if we sanitize the constructors we don't need to do this
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -76,15 +78,23 @@ public enum MetadataBlockType {
static class Contents {
private String _type;
+ private Map<String, String> _stats;
+
+// @JsonCreator
+// public Contents(@JsonProperty("type") String type) {
+// _type = type;
+// }
Review Comment:
why the removal of this?
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/MetadataBlock.java:
##########
@@ -76,15 +78,23 @@ public enum MetadataBlockType {
static class Contents {
private String _type;
+ private Map<String, String> _stats;
+
+// @JsonCreator
+// public Contents(@JsonProperty("type") String type) {
+// _type = type;
+// }
@JsonCreator
- public Contents(@JsonProperty("type") String type) {
+ public Contents(@JsonProperty("type") String type, @JsonProperty("stats")
Map<String, String> stats) {
_type = type;
+ _stats = stats;
}
@JsonCreator
public Contents() {
_type = null;
+ _stats = new HashMap<>();
}
Review Comment:
```suggestion
public Contents() {
this(null, new HashMap<>());
}
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/blocks/TransferableBlock.java:
##########
@@ -55,10 +58,16 @@ public TransferableBlock(List<Object[]> container,
DataSchema dataSchema, DataBl
@VisibleForTesting
TransferableBlock(List<Object[]> container, DataSchema dataSchema,
DataBlock.Type containerType,
boolean isErrorBlock) {
+ this(container, dataSchema, containerType, false, new HashMap<>());
+ }
+
+ public TransferableBlock(List<Object[]> container, DataSchema dataSchema,
DataBlock.Type containerType,
+ boolean isErrorBlock, Map<String, String> metadata) {
_container = container;
_dataSchema = dataSchema;
_type = containerType;
_numRows = _container.size();
+ _metadata = metadata;
Review Comment:
remove. _metadata is not used.
--
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]
