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 8 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
10 changes: 7 additions & 3 deletions google/cloud/bigquery/client.py
View file
Open in desktop
Expand Up @@ -1616,6 +1616,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 +1625,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 +1641,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 Down
7 changes: 6 additions & 1 deletion google/cloud/bigquery/job.py
View file
Open in desktop
Expand Up @@ -1860,7 +1860,6 @@ def destination_encryption_configuration(self):

def to_api_repr(self):
"""Generate a resource for :meth:`_begin`."""

source_refs = [
{
"projectId": table.project,
Expand All @@ -1871,6 +1870,12 @@ def to_api_repr(self):
]

configuration = self._configuration.to_api_repr()
source_table = _helpers._get_sub_prop(
tswast marked this conversation as resolved.
Show resolved Hide resolved
self._configuration._properties, ["copy", "sourceTable"]
)
if source_table:
_helpers._del_sub_prop(configuration, ["copy", "sourceTable"])

_helpers._set_sub_prop(configuration, ["copy", "sourceTables"], source_refs)
_helpers._set_sub_prop(
configuration,
Expand Down
63 changes: 34 additions & 29 deletions tests/unit/test_client.py
View file
Open in desktop
Expand Up @@ -3573,21 +3573,34 @@ 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):
from google.cloud.bigquery import _helpers

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()
if "copy" in job_config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. I'm sure confused by this logic, but I think whatever it was meant to do is unnecessary after #323

if "sourceTable" in job_config["copy"]:
source = _helpers._get_sub_prop(job_config, ["copy", "sourceTable"])
_helpers._del_sub_prop(job_config, ["copy", "sourceTable"])
_helpers._set_sub_prop(job_config, ["copy", "sourceTables"], [source])
if "query" in job_config:
_helpers._del_sub_prop(job_config, ["query", "destinationTable"])
tswast marked this conversation as resolved.
Show resolved Hide resolved

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 +3614,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 +3634,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 +3652,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 +3665,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 +3678,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