Fixed escaping SQL object names #836

larsgeorge-db · 2024-01-24T14:47:24Z

Changes

Implements handling of special characters in SQL names. Optionally escapes the catalog, schema, or table names with backticks for SQL queries.

Linked issues

Resolves #833

Tests

added unit tests

codecov · 2024-01-24T14:49:10Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ae54f76) 85.61% compared to head (f112427) 85.66%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
...rc/databricks/labs/ucx/hive_metastore/locations.py	66.66%	0 Missing and 1 partial ⚠️
src/databricks/labs/ucx/hive_metastore/udfs.py	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
+ Coverage   85.61%   85.66%   +0.05%     
==========================================
  Files          41       42       +1     
  Lines        5212     5232      +20     
  Branches      950      953       +3     
==========================================
+ Hits         4462     4482      +20     
  Misses        536      536              
  Partials      214      214

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dmoore247

Amazing work!
Please consider testing unicode, kanji (Multi-language support, and embedded space chars).

dmoore247 · 2024-01-24T14:57:28Z

tests/unit/framework/test_crawlers.py

+    assert "`a-b`.`c`.`d`" == cb.escape("a-b.c.d", False)
+    assert "`a`.`b-c`.`d`" == cb.escape("a.b-c.d", False)
+    assert "`a`.`b`.`c-d`" == cb.escape("a.b.c-d", False)
+


Amazing!!!
Please consider testing unicode, kanji, embedded space characters.

This documentation isn't much help... https://docs.databricks.com/en/sql/language-manual/sql-ref-names.html#names

nfx · 2024-01-24T15:16:51Z

src/databricks/labs/ucx/framework/crawlers.py

@@ -231,6 +233,28 @@ def _full_name(self) -> str:
        """
        return f"{self._catalog}.{self._schema}.{self._table}"

+    @staticmethod


create a function of it and call it escape_sql_identifier, it doesn't really belong to this class, as it turns out from number of CrawlerBase. prefixes

Yeah, that makes sense, I was about to ask that. Where do we put functions? A new Python source in the root of the project? Or some utils subdir?

daniel-martinez-db · 2024-01-24T21:57:05Z

I have a pattern like this:
my_catalog.my-database.mytable_namev1.0 which should be escaped as
my_catalog.my-database.mytable_name_v1.0
Can that test be included please?

larsgeorge-db · 2024-01-25T13:33:15Z

I have a pattern like this: my_catalog.my-database.mytable_namev1.0 which should be escaped as my_catalog.my-database.mytable_name_v1.0 Can that test be included please?

How would you handle this: cat.alog.schema.table? Is your naming assuming only the last part can have dots in it? Please clarify. Also, see the above linked Databricks Naming convention, it states dots are NOT allowed in names.

daniel-martinez-db · 2024-01-25T14:02:30Z

I have a pattern like this: my_catalog.my-database.mytable_namev1.0 which should be escaped as my_catalog.my-database.mytable_name_v1.0 Can that test be included please?

How would you handle this: cat.alog.schema.table? Is your naming assuming only the last part can have dots in it? Please clarify. Also, see the above linked Databricks Naming convention, it states dots are NOT allowed in names.

Thanks. The patterns I'm working with only has dots in the table name, although I think we can escape all of them (maybe not in the test, but in the actual code). It's true that DB does not allow for dots, but other catalogs do; I'm facing this issue as I'm crawling an external hive metastore which allows for dots. Thank you!

* Added `databricks labs ucx alias` command to create a view of tables from one schema/catalog in another schema/catalog ([#837](#837)). * Added `databricks labs ucx save-aws-iam-profiles` command to scan instance profiles identify AWS S3 access and save a CSV with permissions ([#817](#817)). * Added total view counts in the assessment dashboard ([#834](#834)). * Cleaned up `assess_jobs` and `assess_clusters` tasks in the `assessment` workflow to improve testing and reduce redundancy.([#825](#825)). * Added documentation for the assessment report ([#806](#806)). * Fixed escaping for SQL object names ([#836](#836)). Dependency updates: * Updated databricks-sdk requirement from ~=0.17.0 to ~=0.18.0 ([#832](#832)).

larsgeorge-db requested review from a team and pritishpai January 24, 2024 14:47

larsgeorge-db had a problem deploying to account-admin January 24, 2024 14:47 — with GitHub Actions Failure

dmoore247 approved these changes Jan 24, 2024

View reviewed changes

nfx changed the title ~~Fixed escaping issues with object names.~~ Fixed escaping SQL object names Jan 24, 2024

nfx requested changes Jan 24, 2024

View reviewed changes

larsgeorge-db had a problem deploying to account-admin January 24, 2024 15:35 — with GitHub Actions Failure

larsgeorge-db had a problem deploying to account-admin January 25, 2024 13:54 — with GitHub Actions Error

larsgeorge-db added 3 commits January 25, 2024 14:57

Fixed escaping issues with object names.

c2848f6

Fixed escaping issues with object names.

8dcccfb

Fixed escaping issues with object names.

7ecd12a

larsgeorge-db force-pushed the fix/issue_833_escaping branch from 5f3c84a to 7ecd12a Compare January 25, 2024 13:57

larsgeorge-db had a problem deploying to account-admin January 25, 2024 13:57 — with GitHub Actions Failure

larsgeorge-db requested a review from nfx January 25, 2024 13:57

larsgeorge-db self-assigned this Jan 25, 2024

nfx approved these changes Jan 25, 2024

View reviewed changes

Fixed escaping issues with object names.

f112427

larsgeorge-db had a problem deploying to account-admin January 25, 2024 14:23 — with GitHub Actions Failure

larsgeorge-db temporarily deployed to account-admin January 26, 2024 12:47 — with GitHub Actions Inactive

larsgeorge-db added this pull request to the merge queue Jan 26, 2024

nfx removed this pull request from the merge queue due to a manual request Jan 26, 2024

nfx merged commit bf75b50 into main Jan 26, 2024
7 checks passed

nfx deleted the fix/issue_833_escaping branch January 26, 2024 16:07

nfx mentioned this pull request Jan 26, 2024

Release v0.11.0 #848

Merged

dmoore247 pushed a commit that referenced this pull request Mar 23, 2024

Fixed escaping SQL object names (#836)

8c465b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed escaping SQL object names #836

Fixed escaping SQL object names #836

larsgeorge-db commented Jan 24, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

dmoore247 left a comment

dmoore247 Jan 24, 2024

nfx Jan 24, 2024

larsgeorge-db Jan 25, 2024

daniel-martinez-db commented Jan 24, 2024

larsgeorge-db commented Jan 25, 2024 •

edited

Loading

daniel-martinez-db commented Jan 25, 2024

Fixed escaping SQL object names #836

Fixed escaping SQL object names #836

Conversation

larsgeorge-db commented Jan 24, 2024

Changes

Linked issues

Tests

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

dmoore247 left a comment

Choose a reason for hiding this comment

dmoore247 Jan 24, 2024

Choose a reason for hiding this comment

nfx Jan 24, 2024

Choose a reason for hiding this comment

larsgeorge-db Jan 25, 2024

Choose a reason for hiding this comment

daniel-martinez-db commented Jan 24, 2024

larsgeorge-db commented Jan 25, 2024 • edited Loading

daniel-martinez-db commented Jan 25, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

larsgeorge-db commented Jan 25, 2024 •

edited

Loading