siddharthteotia commented on code in PR #9117:
URL: https://github.com/apache/pinot/pull/9117#discussion_r932616078
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataTable.java:
##########
@@ -123,7 +123,12 @@ enum MetadataKey {
NUM_SEGMENTS_PRUNED_BY_LIMIT("numSegmentsPrunedByLimit",
MetadataValueType.INT),
NUM_SEGMENTS_PRUNED_BY_VALUE("numSegmentsPrunedByValue",
MetadataValueType.INT),
EXPLAIN_PLAN_NUM_EMPTY_FILTER_SEGMENTS("explainPlanNumEmptyFilterSegments",
MetadataValueType.INT),
-
EXPLAIN_PLAN_NUM_MATCH_ALL_FILTER_SEGMENTS("explainPlanNumMatchAllFilterSegments",
MetadataValueType.INT);
+
EXPLAIN_PLAN_NUM_MATCH_ALL_FILTER_SEGMENTS("explainPlanNumMatchAllFilterSegments",
MetadataValueType.INT),
+ // TODO: Add a mechanism to pass generic metadata without adding them to
the DataTable's metadata
+ // This metadata field is only required for emitting a metric to identify
whether queries using MV columns (as
+ // identifiers or via transform) are present in the Selection Order-By
queries. This was the only place where
+ // such a metadata could be added in the existing interfaces even though
we don't need this on the Broker side.
+ QUERY_HAS_MV_SELECTION_ORDER_BY("queryHasMVSelectionOrderBy",
MetadataValueType.STRING);
Review Comment:
I agree. However, the current code is structured in a way that this is the
only mechanism to communicate back such info from operators to QueryScheduler
code in server which emits / logs metrics.
This is a problem as it forces (like in this case) to add a metadata key
when we don't even need one.
Two potential solutions but important to do them cleanly/ right abstraction
- either change APIs in way to propagate down ServerMetrics into operators
(processQuery API ....)
- have a map in DataTable which is not part of serialization but for
transient purpose like above
Thoughts @somandal @Jackie-Jiang
--
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]
