shauryachats commented on code in PR #17464:
URL: https://github.com/apache/pinot/pull/17464#discussion_r2673316575
##########
pinot-timeseries/pinot-timeseries-planner/src/main/java/org/apache/pinot/tsdb/planner/physical/TableScanVisitor.java:
##########
@@ -86,19 +87,20 @@ public void assignSegmentsToPlan(BaseTimeSeriesPlanNode
planNode, TimeBuckets ti
* @param planNode The {@link BaseTimeSeriesPlanNode} to process.
* @return The updated {@link BaseTimeSeriesPlanNode} with table type
information.
*/
- public BaseTimeSeriesPlanNode addTableTypeInfoToPlan(BaseTimeSeriesPlanNode
planNode) {
+ public BaseTimeSeriesPlanNode addTableTypeInfoToPlan(BaseTimeSeriesPlanNode
planNode, RequestContext requestContext) {
if (planNode instanceof LeafTimeSeriesPlanNode) {
LeafTimeSeriesPlanNode sfpNode = (LeafTimeSeriesPlanNode) planNode;
TableRouteInfo routeInfo =
_tableRouteProvider.getTableRouteInfo(sfpNode.getTableName(), _tableCache,
_routingManager);
String tableNameWithType = getTableNameWithType(routeInfo);
Preconditions.checkNotNull(tableNameWithType, "Table not found for table
name: " + sfpNode.getTableName());
+ requestContext.setTableName(tableNameWithType);
Review Comment:
Agree with the philosophy, but do we want to incur the cost of another DFS
just to set the table name already traversed before? Also, I feel storing the
tableName in requestContext is a very small cognitive overhead since this
method was already centered around finding and augmenting the correct table
name.
--
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]
