-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(bigquery/v2): query client based on bigquery/v2 package #12512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
||
// Client is a client for running queries in BigQuery. | ||
type Client struct { | ||
c *apiv2_client.Client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just JobClient so you can choose a single client or the monoclient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when using the Storage integration, the table service is needed. It's used for fetching table schema, which in some cases is not cached, like if the user uses the AttachJob
method
rc *storagepb.BigQueryReadClient | ||
projectID string | ||
billingProjectID string | ||
defaultJobCreationMode bigquerypb.QueryRequest_JobCreationMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To consider: defaultPostRequest rather than a single field, and use proto merge semantics to pick up defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, still need to work on this one.
bigquery/v2/query/query.go
Outdated
} | ||
|
||
// state is one of a sequence of states that a Job progresses through as it is processed. | ||
type state = string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really wish we had a v2 enum to use for this. Since we don't, do we need it at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that enum, is not needed
bigquery/v2/query/query.go
Outdated
Done state = "DONE" | ||
) | ||
|
||
func (q *Query) checkStatus(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this fn name confuses me a bit. The implementation is a bit wonky since it explicitly asks for no rows but appears to have some row cache logic embedded via consumeQueryResponse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to reuse the consumeQueryResponse
method on the newQueryJobFromQueryResponse
method.
} | ||
|
||
// JobReference returns the job reference. | ||
func (q *Query) JobReference() *bigquerypb.JobReference { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't JobReference just a member of the object? What about stateless queries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I can make it a member, but what happens if uses fiddle with it and set it to nil
somehow ? And for stateless queries, the job reference is build using the QueryID
. I need to double check if is working as intended in that part, like trying to call AttachJob
to a stateless query.
bigquery/v2/query/reader.go
Outdated
"cloud.google.com/go/bigquery/v2/apiv2/bigquerypb" | ||
) | ||
|
||
// Reader is used to read the results of a query. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a Query, a Reader, and a RowIterator? This feels like an overlap at a high level, but we can dig in when we talk next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just Query and RowIterators are going to be public, pushed some changes to outline that. Reader
is an interface for reading data using jobs.query
or using Storage Read API.
Draft on new query experience using the new bigquery/v2 client. Will break it down into smaller PR down the line, just testing the interface across languages to settle on a good interface.