Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: broken create_job method #300

Merged
merged 13 commits into from Oct 20, 2020
Merged

fix: broken create_job method #300

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f305f04
fix: broken create_job method
HemangChothani Oct 6, 2020
f98950a
Merge branch 'master' of https://github.com/googleapis/python-bigquer…
HemangChothani Oct 7, 2020
38ce75e
fix: changes in unit tests
HemangChothani Oct 7, 2020
8a27c7c
Merge branch 'master' of https://github.com/googleapis/python-bigquer…
HemangChothani Oct 8, 2020
14bbe01
fix: fix sourceTable thing
HemangChothani Oct 12, 2020
64549c4
Merge branch 'master' of https://github.com/googleapis/python-bigquer…
HemangChothani Oct 12, 2020
f94998f
fix: handle sourceTable passed in job resource
HemangChothani Oct 13, 2020
d8b9e77
Merge branch 'master' of https://github.com/googleapis/python-bigquer…
HemangChothani Oct 13, 2020
b578c90
fix: resolve conflict and address comments
HemangChothani Oct 16, 2020
e130f9a
fix: remove delete destination table from query
HemangChothani Oct 19, 2020
5c7a39b
Merge branch 'master' of https://github.com/googleapis/python-bigquer…
HemangChothani Oct 19, 2020
4223b78
fix: revert destination table for query
HemangChothani Oct 19, 2020
104294a
Merge branch 'master' of https://github.com/googleapis/python-bigquer…
HemangChothani Oct 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions google/cloud/bigquery/client.py
View file
Open in desktop
Expand Up @@ -52,7 +52,6 @@
from google.cloud.bigquery._helpers import _record_field_to_json
from google.cloud.bigquery._helpers import _str_or_none
from google.cloud.bigquery._helpers import _verify_job_config_type
from google.cloud.bigquery._helpers import _del_sub_prop
tswast marked this conversation as resolved.
Show resolved Hide resolved
from google.cloud.bigquery._http import Connection
from google.cloud.bigquery import _pandas_helpers
from google.cloud.bigquery.dataset import Dataset
Expand Down Expand Up @@ -1616,6 +1615,7 @@ def create_job(self, job_config, retry=DEFAULT_RETRY):
)
destination = _get_sub_prop(job_config, ["load", "destinationTable"])
source_uris = _get_sub_prop(job_config, ["load", "sourceUris"])
destination = TableReference.from_api_repr(destination)
return self.load_table_from_uri(
source_uris, destination, job_config=load_job_config, retry=retry
)
Expand All @@ -1624,9 +1624,9 @@ def create_job(self, job_config, retry=DEFAULT_RETRY):
job_config
)
destination = _get_sub_prop(job_config, ["copy", "destinationTable"])
destination = TableReference.from_api_repr(destination)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to guard this check with isinstance(TableReference, destination)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mock further in test so now it cover to_api_repr. I am not sure about the guard to check instance because job_config is dict and as per the example it has values in the string format.

sources = []
source_configs = _get_sub_prop(job_config, ["copy", "sourceTables"])

if source_configs is None:
source_configs = [_get_sub_prop(job_config, ["copy", "sourceTable"])]
tswast marked this conversation as resolved.
Show resolved Hide resolved
for source_config in source_configs:
Expand All @@ -1640,10 +1640,13 @@ def create_job(self, job_config, retry=DEFAULT_RETRY):
job_config
)
source = _get_sub_prop(job_config, ["extract", "sourceTable"])
source_type = "Table"
if not source:
if source:
source_type = "Table"
source = TableReference.from_api_repr(source)
else:
source = _get_sub_prop(job_config, ["extract", "sourceModel"])
source_type = "Model"
source = ModelReference.from_api_repr(source)
destination_uris = _get_sub_prop(job_config, ["extract", "destinationUris"])
return self.extract_table(
source,
Expand All @@ -1654,7 +1657,6 @@ def create_job(self, job_config, retry=DEFAULT_RETRY):
)
elif "query" in job_config:
copy_config = copy.deepcopy(job_config)
_del_sub_prop(copy_config, ["query", "destinationTable"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize we were already removing this property in past releases. We should restore this, since I don't want to break existing clients.

query_job_config = google.cloud.bigquery.job.QueryJobConfig.from_api_repr(
copy_config
)
Expand Down
62 changes: 27 additions & 35 deletions tests/unit/test_client.py
View file
Open in desktop
Expand Up @@ -3573,21 +3573,24 @@ def test_delete_table_w_not_found_ok_true(self):

conn.api_request.assert_called_with(method="DELETE", path=path, timeout=None)

def _create_job_helper(self, job_config, client_method):
def _create_job_helper(self, job_config):
creds = _make_credentials()
http = object()
client = self._make_one(project=self.PROJECT, credentials=creds, _http=http)

client._connection = make_connection()
rf1 = mock.Mock()
get_config_patch = mock.patch(
"google.cloud.bigquery.job._JobConfig.from_api_repr", return_value=rf1,
)
load_patch = mock.patch(client_method, autospec=True)
RESOURCE = {
"jobReference": {"projectId": self.PROJECT, "jobId": mock.ANY},
"configuration": job_config,
}
conn = client._connection = make_connection(RESOURCE)
client.create_job(job_config=job_config)

with load_patch as client_method, get_config_patch:
client.create_job(job_config=job_config)
client_method.assert_called_once()
conn.api_request.assert_called_once_with(
method="POST",
path="/projects/%s/jobs" % self.PROJECT,
data=RESOURCE,
timeout=None,
)

def test_create_job_load_config(self):
configuration = {
Expand All @@ -3601,9 +3604,7 @@ def test_create_job_load_config(self):
}
}

self._create_job_helper(
configuration, "google.cloud.bigquery.client.Client.load_table_from_uri"
)
self._create_job_helper(configuration)

def test_create_job_copy_config(self):
configuration = {
Expand All @@ -3623,9 +3624,7 @@ def test_create_job_copy_config(self):
}
}

self._create_job_helper(
configuration, "google.cloud.bigquery.client.Client.copy_table",
)
self._create_job_helper(configuration)

def test_create_job_copy_config_w_single_source(self):
tswast marked this conversation as resolved.
Show resolved Hide resolved
configuration = {
Expand All @@ -3643,9 +3642,7 @@ def test_create_job_copy_config_w_single_source(self):
}
}

self._create_job_helper(
configuration, "google.cloud.bigquery.client.Client.copy_table",
)
self._create_job_helper(configuration)

def test_create_job_extract_config(self):
configuration = {
Expand All @@ -3658,9 +3655,7 @@ def test_create_job_extract_config(self):
"destinationUris": ["gs://test_bucket/dst_object*"],
}
}
self._create_job_helper(
configuration, "google.cloud.bigquery.client.Client.extract_table",
)
self._create_job_helper(configuration)

def test_create_job_extract_config_for_model(self):
configuration = {
Expand All @@ -3673,17 +3668,17 @@ def test_create_job_extract_config_for_model(self):
"destinationUris": ["gs://test_bucket/dst_object*"],
}
}
self._create_job_helper(
configuration, "google.cloud.bigquery.client.Client.extract_table",
)
self._create_job_helper(configuration)

def test_create_job_query_config(self):
configuration = {
"query": {"query": "query", "destinationTable": {"tableId": "table_id"}}
"query": {
"query": "query",
"destinationTable": {"tableId": "table_id"},
"useLegacySql": False,
}
}
self._create_job_helper(
configuration, "google.cloud.bigquery.client.Client.query",
)
self._create_job_helper(configuration)

def test_create_job_query_config_w_rateLimitExceeded_error(self):
from google.cloud.exceptions import Forbidden
Expand Down Expand Up @@ -3711,9 +3706,9 @@ def test_create_job_query_config_w_rateLimitExceeded_error(self):
}
},
}
data_without_destination = {
data = {
"jobReference": {"projectId": self.PROJECT, "jobId": mock.ANY},
"configuration": {"query": {"query": query, "useLegacySql": False}},
"configuration": configuration,
}

creds = _make_credentials()
Expand All @@ -3740,10 +3735,7 @@ def test_create_job_query_config_w_rateLimitExceeded_error(self):
self.assertEqual(
fake_api_request.call_args_list[1],
mock.call(
method="POST",
path="/projects/PROJECT/jobs",
data=data_without_destination,
timeout=None,
method="POST", path="/projects/PROJECT/jobs", data=data, timeout=None,
),
)

Expand Down