Add ResolveBaseDatabricksFullTableName to DataCatalogue - #37
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new public API method naming and multiple user-facing error messages are inconsistent/misleading and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a DataCatalogue helper to resolve the underlying Unity Catalog external table for a write-shared Databricks view by reading UC information schema metadata and tags, with accompanying unit tests to validate the resolution flow and guard against SQL string interpolation.
Changes:
- Introduces
DataCatalogue.ResolveBaseDatabricksFullTableName()that validates a fully-qualified view name, verifies it is a plain view, readssource_dataset_id, and resolves the correspondingdataset_id-tagged external table. - Adds a new test suite covering invalid inputs, missing metadata/tag scenarios, non-external base types, successful resolution, and parameterized SQL usage.
File summaries
| File | Description |
|---|---|
src/dataworkbench/datacatalogue.py |
Adds the base-table resolution method backed by system.information_schema queries and tag lookups. |
tests/test_datacatalogue.py |
Adds unit tests for base-table resolution behavior and SQL parameterization expectations. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+241
to
+243
| raise ValueError( | ||
| "the base for this view is not a table. Invalid viewName given as input" | ||
| ) |
dorislinda
reviewed
Sep 4, 2026
Renamed to resolve_base_databricks_full_table_name so the public API matches the snake_case used by the rest of the module. The error raised when the base object is not EXTERNAL said "is not a table", which was misleading for a MANAGED base -- it is a table, just not the one the method accepts. It now names the real constraint. The log call takes arguments instead of an f-string, so the message is only formatted if the level is enabled. The returned name is backtick quoted per identifier, doubling any embedded backtick the way BuildNameForSelectClause does on the service side, so the result can be dropped straight into Spark SQL. The WHERE clauses are unchanged: they bind through spark.sql(args=...), which hands literals to the JVM rather than substituting into the query text, so escaping quotes there would corrupt the lookup instead of protecting it. A test pins that behaviour.
dorislinda
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.