[iennae]

… upgrade a manifest external table to BigLake with automatic metadata caching

Problem:
The BigQuery documentation (https://cloud.google.com/bigquery/docs/query-open-table-format-using-manifest-files) needs a standalone, tested Python code sample demonstrating how to upgrade a manifest external table to BigLake with automatic metadata caching (region tag bigquery_upgrade_manifest_biglake_table).

Solution:

• Add upgrade_manifest_biglake_table(project_id: str, dataset_id: str, table_id: str, connection_id: str, manifest_uri: str) -> None in bigquery/biglake/bigquery_upgrade_manifest_biglake_table.py (Upgrade a manifest-backed external table to a BigLake table with AUTOMATIC metadata caching and connection binding.)
• Add unit tests (bigquery/biglake/bigquery_upgrade_manifest_biglake_table_test.py) covering both the success path and API error handling (GoogleAPICallError)

Description

Fixes #

Checklist

Testing

• I have tested this change on a live environment and verified it works as intended.

Compliance & Style

• I have followed the Sample Guidelines.
• The README is updated with all relevant information (if applicable).
• If this PR adds a new sample directory: I updated the CODEOWNERS file.

---

Post-Approval Actions

• Please merge this PR for me once it is approved

[snippet-bot]

Here is the summary of changes.

You are about to add 1 region tag.

• bigquery/biglake/bigquery_upgrade_manifest_biglake_table.py:15, tag bigquery_upgrade_manifest_biglake_table

---

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment

[gemini-code-assist]

Code Review

This pull request introduces a new Python sample and associated tests for upgrading a manifest external table to a BigLake table with metadata caching in BigQuery. Feedback on the implementation suggests making the verification query schema-agnostic by using SELECT * instead of hardcoded column names. Additionally, the test configuration in noxfile_config.py should be updated to include Python 3.10 in the test suite, as it is the oldest supported version and should not be ignored.

[gemini-code-assist]

The SELECT query hardcodes specific column names (id, label, score). Since this is a generic code sample designed to be run against any manifest-backed external table, hardcoding these columns will cause the query to fail if the user's Parquet files have a different schema. Consider using SELECT * to make the query schema-agnostic and reusable.

Suggested change

	SELECT
	id,
	label,
	score
	FROM `{table_ref}`
	ORDER BY id ASC
	LIMIT 10;
	SELECT * FROM {table_ref} LIMIT 10;

[gemini-code-assist]

According to the repository guidelines, we should only test the oldest and newest supported Python releases in test configurations, ignoring intermediate versions. Since Python 3.10 is the oldest supported version, it should not be ignored. Please remove '3.10' from ignored_versions so that it is included in the test suite.

Suggested change

	"ignored_versions": ["2.7", "3.7", "3.8", "3.9", "3.10", "3.11"],
	"ignored_versions": ["2.7", "3.7", "3.8", "3.9", "3.11"],

References

1. Only test the oldest and newest supported Python releases in test configurations (such as noxfile_config.py), ignoring intermediate versions.

[chalmerlowe]

@iennae

Thinking out loud

My understanding is that there is a misconception about multi-part SQL statements.

You prolly know this, but in case others come behind us I will provide some context. Some folks believe that multi-part SQL statements don't return results the way we might expect. For example if I included some form of Data Definition Language (DDL) statement like DROP TABLE after the SELECT, we would not get back the results of the SELECT, we would get back an empty list.

This is because:

• Multi-statement queries are execute as a statement_type = "SCRIPT"
• In the ScriptOptions object, the key_result_statement property is defined, by default using the enum: KeyResultStatementKind.LAST

For this reason, I wonder if there is merit to separating the DDL portion (CREATE OR REPLACE EXTERNAL TABLE) of this example from the Data Query Language (DQL) portion (SELECT) as part of demonstrating good hygiene.

Something like this so they can be called separately:

# 1. Create or replace the BigLake external table
client.query(ddl_sql).result()

# 2. Query the newly created external table
query_job = client.query(select_sql)
rows = list(query_job.result())

Pros: two calls makes it easier to do troubleshooting if one of the major steps fails. Searching audit logs, job history, etc should be easier especially since customers won't have to drill down into parent jobs to find child jobs.
Cons: There is a small performance hit when a client.query() provisions and registers each top-level job.

[chalmerlowe]

COMMENT

I looked at about seven samples. The ones I saw all appear to have this try/except/except pattern.

I am worried that this degrades the signal-to-noise ratio in our reference samples, documentation, and quickstarts.

I know the Samples Style Guide says to include Error Handling.

The catch blocks take up roughly 20 lines of repetitive boilerplate code that prints to stderr and exits early. I wonder if this could be handled differently.

[chalmerlowe]

Just thinking out loud

In PRs #14740 and #14748, string-target patching is used (e.g. mock.patch("google.cloud.bigquery.Client")). If the sample refactors how Client is imported, string targets can silently break or patch the wrong namespace. (I understand that it is not likely that these samples will be wildly refactored - hence just thinking out loud. I don't think there is a huge ROI in inducing review churn over this.)

To keep target-safety while retaining mock's auto-generated mock instances and call assertions, there may be utility in using mock.patch.object(module.bigquery, "Client") instead of raw string paths.

Alternatively, if we want to match the other PRs, you could use monkeypatch.setattr with a MagicMock.

[XrossFox]

I think there's no point in printing the metadata cache mode here, because it's a hardcoded value and the response QueryObject doesnt include wether the table created is using metadata cache, it would make more sense to use a get_table() call to inspect the newly created table and inspect the resulted Table object TableObject.external_data_configuration.metadata_cache_mode and print it's value, then it could be asserted during testing.

[XrossFox]

It'd probably be better to patch bigquery_upgrade_manifest_biglage_table.client instance, rather than patch the library.