[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9117: Emit a server side metric to identify if MV columns are present as part of the selection order by queries

[GitBox <gi...@apache.org>]

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. 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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org