Conversation
|
@kevinjqliu when you have a chance, could you please take a look at this pr and let me know if any update is needed? |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
…oad_namespace_properties
BigQueryMetastoreCatalog.create_namespace stores user properties in the
ExternalCatalogDatasetOptions.parameters map, but load_namespace_properties
returned external_catalog_dataset_options.to_api_repr(). That API
representation is a nested dict that buries the properties under a
"parameters" key and adds "defaultStorageLocationUri", so a round-trip of
create_namespace(ns, {"owner": "..."}) followed by
load_namespace_properties(ns)["owner"] raised KeyError.
Return the flat parameters map instead, matching the base MetastoreCatalog
contract and the Glue and DynamoDB catalogs. Guard the None case (parameters
is unset by default) so a namespace with no properties yields an empty dict.
Add regression tests covering both the populated round-trip and the
no-parameters case.
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
c334954 to
cbb1a9f
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change is covered by regression tests and has no blocking issues.
Pull request overview
Updates BigQuery namespace property loading to return flat properties consistent with the catalog contract.
Changes:
- Returns namespace parameters as a flat map.
- Adds tests for populated and unset parameters.
File summaries
| File | Description |
|---|---|
tests/catalog/test_bigquery_metastore.py |
Adds regression coverage for namespace properties. |
pyiceberg/catalog/bigquery_metastore.py |
Returns flat namespace properties. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The base
MetastoreCatalogcontract types this method as-> Properties(a flatDict[str, str]), and the Glue and DynamoDB catalogs both return the flat map(
dict(database.get("Parameters", {}))). BigQuery was the outlier. This changesit to return the flat
parametersmap, using the same.parametersaccessoralready used on the table path in this file. The
parametersfield isNonewhen unset, so it is guarded to yield an empty dict for a namespace with no
properties.
Are these changes tested?
Yes. Two regression tests were added to
tests/catalog/test_bigquery_metastore.py:test_load_namespace_properties_returns_flat_parameters: creates a namespacewith
{"owner": ..., "comment": ...}, then assertsload_namespace_propertiesreturns exactly that flat map (the mock feeds the persisted
Datasetbackthrough
get_dataset). This fails on the oldto_api_repr()code (returns thenested dict) and passes with the fix.
test_load_namespace_properties_without_parameters: asserts a dataset whoseoptions carry no parameters yields
{}rather thanNoneor a nested dict.python -m pytest tests/catalog/test_bigquery_metastore.py-> 7 passed. Lint(
ruff,ruff-format,mypy,pydocstyle,codespell) passes on both files.The BigQuery integration tests need Docker + Spark, which were not available in
this environment; the behavior is covered by the unit tests above.
Are there any user-facing changes?
Yes.
BigQueryMetastoreCatalog.load_namespace_propertiesnow returns the flatnamespace property map, consistent with the other catalogs, instead of the nested
BigQuery API representation. Code that relied on the previous nested shape
(reading
["parameters"][...]or["defaultStorageLocationUri"]) would need toread the properties directly; the previous shape did not match the documented
Propertiesreturn type.