Why PlanNodeStats has different number of CpuWallTiming variables compare to OperatorStats

[Yohahaha]

Description

Each OperatorStats has three kinds of timers, which are addInputTiming, getOutputTiming, finishTiming. However, PlanNodeStats only has one cpuWallTiming. When task is done, OperatorStats will be merged into PlanNodeStats, then we lost fine-grained metrics for these different stage's timing.

velox/velox/exec/PlanNodeStats.cpp

Lines 46 to 48 in e4a2c99

	cpuWallTiming.add(stats.addInputTiming);
	cpuWallTiming.add(stats.getOutputTiming);
	cpuWallTiming.add(stats.finishTiming);

Fine-grained metrics is important for some operator, for example, we could know the elapsed time for Agg to build hash table by addInputTiming.

I am curious about the design for this difference, and it would be great that using same number of timer in PlanNodeStats to consistent with OperatorStats.

I would like to contribute a pr for this, if you agree this idea, thanks.

[Yohahaha]

@mbasmanova would you help take a look?

[pedroerp]

Just moving the Issue to a Discussion to get more feedback. Other than Masha (she's off this week), maybe @xiaoxmeng @spershin @majetideepak @aditi-pandit could also give some feedback?

[mbasmanova]

@Yohahaha In principle these finer grained stats might be useful, but over the years we haven't found this to be case. We also noticed that it difficult to use the stats if only fine-grained stats are available as we always found ourselves writing code to add them up. That said, I see no problem with keeping total stat (cpuWallTiming) and adding fine-grained stats.

[Yohahaha]

Thanks for reply.