feat(bigquery): expose StatementType and query execution stats on TableResult - #14145
feat(bigquery): expose StatementType and query execution stats on TableResult#14145keshavdandeva wants to merge 2 commits into
StatementType and query execution stats on TableResult#14145Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the BigQuery dependency version and propagates query execution statistics—including statement type, total bytes billed, total bytes processed, total slot milliseconds, and affected DML rows—from query results and job statistics into the TableResult object. The review feedback suggests wrapping the StatementType.valueOf() conversion in a try-catch block to gracefully handle any unrecognized statement types returned by the BigQuery service and prevent runtime exceptions.
|
@keshavdandeva FYI, I'm going to upgrade the bigquery apiary version in #14149 |
| QueryStatistics stats = | ||
| job.getStatistics() instanceof QueryStatistics | ||
| ? (QueryStatistics) job.getStatistics() | ||
| : null; |
There was a problem hiding this comment.
From what I see in the JobStatistic, there are quite a few of impl classes:
- CopyStatistics
- JobStatistics
- Load
- Query
- ...
Is there any concern if we just return the whole JobStatistic impl class back? Downstream user (e.g. bigquery-jdbc) can contain the logic to parse to QueryStatistic and pull the relevant fields. This way we don't need to maintain the mapping of select fields for QueryStatistic.
There was a problem hiding this comment.
Yeah so, I looked into using JobStatistics / QueryStatistics, and wanted to share a couple of thoughts/questions to get your take:
- For fast-path/jobless queries, the backend doesn't return a
Jobor aJobStatisticspayload; it only returns these specific execution fields directly onQueryResponse. If we populate aJobStatistics.QueryStatisticsobject fromQueryResponse, most of its fields (queryPlan,timeline,referencedTables,billingTier, etc.) would remain empty/null. Idk if that would be confusing or misleading for callers expecting a full Job statistics object? - Since
TableResultis only ever produced by query executions (not Copy/Load/Extract), and already exposes direct metadata getters likegetSchema(),getQueryId(), andgetJobCreationReason(), adding direct getters seemed to align with the existing pattern while keeping it strongly typed and avoiding downstream downcasting.
What do you think? If you feel wrapping these in QueryStatistics on TableResult is still the better direction for the SDK, I'm more than happy to update it
There was a problem hiding this comment.
For fast-path/jobless queries, the backend doesn't return a Job or a JobStatistics payload; it only returns these specific execution fields directly on QueryResponse. If we populate a JobStatistics.QueryStatistics object from QueryResponse, most of its fields (queryPlan, timeline, referencedTables, billingTier, etc.) would remain empty/null. Idk if that would be confusing or misleading for callers expecting a full Job statistics object?
Can you show me what you mean by specific execution fields directly on QueryResponse? I don't think I see this logic in the PR and I'm not sure I'm wrapping my head around this part.
I also don't think we want to manually populate JobStatistics object. We should try to only propagate server response back to user, no need for us to do any special transformation or logic (just convert to and from PBs and return null if the server gives us null). Do we have documentation that tells users that they would always be expecting a full Job statistics object even for fast path?
Since TableResult is only ever produced by query executions (not Copy/Load/Extract), and already exposes direct metadata getters like getSchema(), getQueryId(), and getJobCreationReason(), adding direct getters seemed to align with the existing pattern while keeping it strongly typed and avoiding downstream downcasting.
IIUC, TableResult is returned from the Query RPC. Anyone can just call the CreateJob RPC with a copy/load/extract operation. Unless there is strict requirement, I think a generic Job interface should return a generic JobStatistic result (responsibility would be on downstream user to figure out the type of jobstatistic and pull out the relevant info)
There was a problem hiding this comment.
Yeah, that makes sense, and I'll update TableResult to return JobStatistics
Just to answer your questions on why I initially structured it with individual getters:
- In
BigQueryImpl.java(lines 2099–2106),resultsiscom.google.api.services.bigquery.model.QueryResponsereturned byqueryRpc. The generated model has these fields directly at the top level (results.getStatementType(),results.getTotalBytesBilled(), etc.) rather than inside a nested statistics object. - Because
QueryResponsehad them as top-level getters, I had initially mapped them 1-to-1 directly ontoTableResult(similar togetSchema()andgetQueryId()). - You're right that there is no documentation requiring full statistics on the fast path, so populating
JobStatistics.QueryStatisticswith the available metrics fromQueryResponseworks well.
b333559 to
94fbc07
Compare
b/549680449
This PR exposes
StatementTypeand jobless query execution metrics onTableResultby plumbing them fromQueryResponseandJobStatistics.QueryStatistics.This enables downstream consumers (such as the BigQuery JDBC driver) to inspect query statement types and execution statistics directly from
TableResultwithout needing to issue secondary jobs or dry-run queries.Changes
google-api-services-bigquerytov2-rev20260731-2.0.0ingoogle-cloud-jar-parent/pom.xmlandjava-bigquery/pom.xml.getNextPage()),toString(),hashCode(), andequals()for:getStatementType()(StatementType)getTotalBytesBilled()(Long)getTotalBytesProcessed()(Long)getTotalSlotMs()(Long)getNumDmlAffectedRows()(Long)QueryResponsewhen buildingTableResultinqueryRpc().StatementTypeand query metrics fromQueryStatisticswhen creatingTableResultingetQueryResults().TableResultTestandBigQueryImplTestverifying field retrieval, pagination propagation, builder modifications, and serialization.