make explain return planner metrics as separate column #3064
Conversation
Result of fdb-record-layer-pr on Linux CentOS 7
|
| @@ -318,6 +318,9 @@ public QueryPlanResult planQuery(@Nonnull final RecordQuery query, @Nonnull Para | |||
| .put(QueryPlanInfoKeys.TOTAL_TASK_COUNT, taskCount) | |||
| .put(QueryPlanInfoKeys.MAX_TASK_QUEUE_SIZE, maxQueueSize) | |||
| .put(QueryPlanInfoKeys.CONSTRAINTS, constraints) | |||
| .put(QueryPlanInfoKeys.STATS_MAPS, | |||
| Debugger.getDebuggerMaybe().flatMap(Debugger::getStatsMaps) | |||
There was a problem hiding this comment.
I take it that the query plan result won't be returned if the STATS_MAP is empty if the debugger isn't present? Maybe that's fine, as this information is a little internal. It does potentially limit the ability to get these stats through the relational server, but maybe we don't care about that (at least for the moment)
| @@ -273,12 +282,23 @@ private RelationalResultSet executeExplain(@Nonnull ContinuationImpl parsedConti | |||
| FieldDescription.primitive("VERSION", Types.INTEGER, DatabaseMetaData.columnNoNulls), | |||
| FieldDescription.primitive("PLAN_HASH_MODE", Types.VARCHAR, DatabaseMetaData.columnNoNulls) | |||
| ); | |||
| StructMetaData plannerMetricsMetadata = new RelationalStructMetaData( | |||
| FieldDescription.primitive("TASK_COUNT", Types.BIGINT, DatabaseMetaData.columnNoNulls), | |||
There was a problem hiding this comment.
Do you know if we have coverage of what happens if there isn't a debugger set (e.g., making sure that we don't return a null column here)? This looks right to me, but I just wanted to see if it's covered.
There was a problem hiding this comment.
There are plenty of tests where we don't have a Debugger installed (all that's not EXPLAIN) and this is null
| final var expectedLabels = List.of("PLAN", "PLAN_HASH", "PLAN_DOT", "PLAN_GML", "PLAN_CONTINUATION"); | ||
| final var expectedTypes = List.of(Types.VARCHAR, Types.INTEGER, Types.VARCHAR, Types.VARCHAR, Types.STRUCT); | ||
| final var expectedLabels = List.of("PLAN", "PLAN_HASH", "PLAN_DOT", "PLAN_GML", "PLAN_CONTINUATION", "PLANNER_METRICS"); | ||
| final var expectedTypes = List.of(Types.VARCHAR, Types.INTEGER, Types.VARCHAR, Types.VARCHAR, Types.STRUCT, Types.STRUCT); |
There was a problem hiding this comment.
I'm not sure it needs to be here, but it would be nice to have a test somewhere that asserted on the values of the planner stats struct, even if it was just making sure the values weren't null or zero. Getting a more sophisticated test set of assertions is probably for another time.
There was a problem hiding this comment.
I added a few verifys to the planGraph API in record-layer-core and I added some verifys in relational-core in QueryConfig. There is more code coming in that uses these metrics.
| final var plannerMetrics = resultSet.getStruct(6); | ||
| if (plannerMetrics != null) { |
There was a problem hiding this comment.
Should the plannerMetrics ever be null? I'd think that it should always be non-null except maybe for mixed mode tests where the column wouldn't be defined (and I guess must not be running here because it's happening in the explain step)
There was a problem hiding this comment.
It's null if the server does not have a Debugger installed.
normen662
force-pushed
the
metrics-in-explain
branch
from
41fa34a to
2f565f5
Compare
Result of fdb-record-layer-pr on Linux CentOS 7
|
Most of the PR is just making sure that the
Debuggerstats structures become accessible to the code that prepares and packages the result that is sent to the client. For that purpose I extractedStatsfrom the respectiveDebuggerimplementations and make it is own first-class citizen. Similar goes forStatsMaps. One thing I wanted to make sure is that once theStatsMapsare accessed outside of the installedDebugger, theStatsMapsbecome immutable. For that purpose we make a copy, as usual, however, we only want to make that copy when the client of the planner (i.e. QueryPlan) needs the stats (when responding to anEXPLAIN).