Add SQL query planning time metric #12923
Conversation
rohangarg
force-pushed
the
sqlPlanningMetrics
branch
from
979d82f
to
4c7309d
Compare
|
|
||
| final Map<String, Object> statsMap = new LinkedHashMap<>(); | ||
| statsMap.put("sqlQuery/time", TimeUnit.NANOSECONDS.toMillis(queryTimeNs)); | ||
| statsMap.put("sqlQuery/planningTimeMs", TimeUnit.NANOSECONDS.toMillis(planningTimeNanos)); |
There was a problem hiding this comment.
I'm curious that if this metric is emitted in milliseconds, why do we need to hold it in nanosecond in the code?
There was a problem hiding this comment.
I guess that it's because of the better accuracy.
2089/1000 - 1989/1000 = 1
(2089 - 1989) / 1000 = 0
0 seems better here given the minuscule difference between two values.
There was a problem hiding this comment.
Also, my thinking was that given it doesn't need any extra space, it is better to store the time in the highest resolution. Then we can emit in whatever resolution is needed by the callers and also use the highest accuracy times for any arithmetic if needed in future.
There was a problem hiding this comment.
But I don't see if there's any chance that we might change this metric from ms to ns in the future since that would cause incompatibility.
I think SQL planning phase should be very short, does this phase always take milliseconds to complete? I guess this phase should be done in micro-seconds, if it's true, the emitted metric is always zero in most cases.
There was a problem hiding this comment.
Yes, I didn't mean to say that we'd update the current metric. Rather that incase anyone tries to use it for a new metric or derive a metric using the planningTime, they would have the value with highest accuracy.
In my local tests with SELECT 1 query, even after multiple quick invocations the lowest planning time I saw was 4ms. The lowest query/time observed was 6ms. Have you seen < millisecond scale planning in production clusters maybe - which my local test would be missing?
There was a problem hiding this comment.
FWIW, I have seen many queries taking significant planning time. For E.g. we have seen this in particular with queries with a large IN clause or queries with many UNION operators. When debugging a slow query, we often don't have direct information about the time spent in planning. We tend to infer it by looking at other metrics and then comparing it with overall execution time. Having this metric available firsthand would help in that troubleshooting. If that planning time is too high, we would either fix it through a code change or adjust the query in some way.
So IMO there is definitely utility in having this metric.
There was a problem hiding this comment.
@rohangarg I just guess based on my experience, and don't have evidence to prove that.
Adds
sqlQuery/planningTimeMsmetric per SQL query which computes the time it takes to build a native query from the SQL query.While comparing the perf of native to SQL API for the same query, users tend to check the extra time taken by planning stage of the SQL. Currently, that time is estimated as the difference between running the SQL and then running the native query using EXPLAIN. To find the exact difference, the
query/timemetrics of both SQL and native queries are compared.Further, for SQL query workloads there is no easy visiblity into the holistic planning time.
This PR has: