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.

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

Added TableMapping functionality to table migrate #752

Merged
merged 6 commits into from
Jan 4, 2024

Conversation

FastLee
Copy link
Contributor

@FastLee FastLee commented Jan 3, 2024

No description provided.

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

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

Comparison is base (06c666c) 80.04% compared to head (df13e6a) 80.18%.

Files Patch % Lines
...atabricks/labs/ucx/hive_metastore/table_migrate.py 81.63% 8 Missing and 1 partial ⚠️
src/databricks/labs/ucx/cli.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #752      +/-   ##
==========================================
+ Coverage   80.04%   80.18%   +0.14%     
==========================================
  Files          44       44              
  Lines        4610     4628      +18     
  Branches      859      856       -3     
==========================================
+ Hits         3690     3711      +21     
+ Misses        696      694       -2     
+ Partials      224      223       -1     

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

Copy link
Collaborator

@nfx nfx left a comment

Choose a reason for hiding this comment

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

revert moving public/private methods around, it's harder to review

src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
tests/integration/hive_metastore/test_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/tables.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/mapping.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/cli.py Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/mapping.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
@nfx
Copy link
Collaborator

nfx commented Jan 3, 2024

test

src/databricks/labs/ucx/hive_metastore/mapping.py Outdated Show resolved Hide resolved
rules.append(Rule(**row))
skip_table = row.get("skip_table")
row["skip_table"] = skip_table and skip_table == "true"
rules.append(Rule(**row)) # type: ignore[arg-type]
Copy link
Collaborator

@nfx nfx Jan 4, 2024

Choose a reason for hiding this comment

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

Suggested change
rules.append(Rule(**row)) # type: ignore[arg-type]
rules.append(Rule(skip_table=row.get("skip_table", "false") == "true", **row))

# type: ignore is not allowed until you write 200k lines of code in python in public repositories ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed skip altogether. Skip will be handled in next PR as part of the Mapping class.

src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
src/databricks/labs/ucx/hive_metastore/table_migrate.py Outdated Show resolved Hide resolved
Comment on lines 39 to 43
tasks.append(partial(self._migrate_table, table, rule.as_uc_table_key))
Threads.strict("migrate tables", tasks)

def _migrate_table(self, src_table: Table, target_table_key):
sql = src_table.uc_create_sql(target_table_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tasks.append(partial(self._migrate_table, table, rule.as_uc_table_key))
Threads.strict("migrate tables", tasks)
def _migrate_table(self, src_table: Table, target_table_key):
sql = src_table.uc_create_sql(target_table_key)
tasks.append(partial(self._migrate_table, table, rule))
Threads.strict("migrate tables", tasks)
def _migrate_table(self, src_table: Table, rule: Rule):
target_table_key = rule.as_uc_table_key
sql = src_table.uc_create_sql(target_table_key)

@nfx nfx merged commit bba54e5 into main Jan 4, 2024
7 checks passed
@nfx nfx deleted the feature/table_mapping_migrate branch January 4, 2024 16:17
@nfx
Copy link
Collaborator

nfx commented Jan 9, 2024

test comment...

nfx added a commit that referenced this pull request Jan 12, 2024
* Added assessment step to estimate the size of DBFS root tables ([#741](#741)).
* Added `TableMapping` functionality to table migrate ([#752](#752)).
* Added `databricks labs ucx move` command to move tables and schemas between catalogs ([#756](#756)).
* Added functionality to determine migration method based on DBFS Root ([#759](#759)).
* Added `get_tables_to_migrate` functionality in the mapping module ([#755](#755)).
* Added retry and rate limit to rename workspace group operation and corrected rate limit for reflecting account groups to workspace ([#751](#751)).
* Adopted `databricks-labs-blueprint` library for common utilities to be reused in the other projects ([#758](#758)).
* Converted `RuntimeBackend` query executions exceptions to SDK exceptions ([#769](#769)).
* Fixed issue with missing users and temp groups after workspace-local groups migration and skip table when crawling table size if it does not exist anymore ([#770](#770)).
* Improved error handling by not failing group rename step if a group was removed from account before reflecting it to workspace ([#762](#762)).
* Improved error message inference from failed workflow runs ([#753](#753)).
* Moved `TablesMigrate` to a separate module ([#747](#747)).
* Reorganized assessment dashboard to increase readability ([#738](#738)).
* Updated databricks-sdk requirement from ~=0.16.0 to ~=0.17.0 ([#773](#773)).
* Verify metastore exists in current workspace ([#735](#735)).
@nfx nfx mentioned this pull request Jan 12, 2024
nfx added a commit that referenced this pull request Jan 12, 2024
* Added assessment step to estimate the size of DBFS root tables
([#741](#741)).
* Added `TableMapping` functionality to table migrate
([#752](#752)).
* Added `databricks labs ucx move` command to move tables and schemas
between catalogs
([#756](#756)).
* Added functionality to determine migration method based on DBFS Root
([#759](#759)).
* Added `get_tables_to_migrate` functionality in the mapping module
([#755](#755)).
* Added retry and rate limit to rename workspace group operation and
corrected rate limit for reflecting account groups to workspace
([#751](#751)).
* Adopted `databricks-labs-blueprint` library for common utilities to be
reused in the other projects
([#758](#758)).
* Converted `RuntimeBackend` query executions exceptions to SDK
exceptions ([#769](#769)).
* Fixed issue with missing users and temp groups after workspace-local
groups migration and skip table when crawling table size if it does not
exist anymore ([#770](#770)).
* Improved error handling by not failing group rename step if a group
was removed from account before reflecting it to workspace
([#762](#762)).
* Improved error message inference from failed workflow runs
([#753](#753)).
* Moved `TablesMigrate` to a separate module
([#747](#747)).
* Reorganized assessment dashboard to increase readability
([#738](#738)).
* Updated databricks-sdk requirement from ~=0.16.0 to ~=0.17.0
([#773](#773)).
* Verify metastore exists in current workspace
([#735](#735)).
FastLee pushed a commit that referenced this pull request Jan 19, 2024
* Added assessment step to estimate the size of DBFS root tables
([#741](#741)).
* Added `TableMapping` functionality to table migrate
([#752](#752)).
* Added `databricks labs ucx move` command to move tables and schemas
between catalogs
([#756](#756)).
* Added functionality to determine migration method based on DBFS Root
([#759](#759)).
* Added `get_tables_to_migrate` functionality in the mapping module
([#755](#755)).
* Added retry and rate limit to rename workspace group operation and
corrected rate limit for reflecting account groups to workspace
([#751](#751)).
* Adopted `databricks-labs-blueprint` library for common utilities to be
reused in the other projects
([#758](#758)).
* Converted `RuntimeBackend` query executions exceptions to SDK
exceptions ([#769](#769)).
* Fixed issue with missing users and temp groups after workspace-local
groups migration and skip table when crawling table size if it does not
exist anymore ([#770](#770)).
* Improved error handling by not failing group rename step if a group
was removed from account before reflecting it to workspace
([#762](#762)).
* Improved error message inference from failed workflow runs
([#753](#753)).
* Moved `TablesMigrate` to a separate module
([#747](#747)).
* Reorganized assessment dashboard to increase readability
([#738](#738)).
* Updated databricks-sdk requirement from ~=0.16.0 to ~=0.17.0
([#773](#773)).
* Verify metastore exists in current workspace
([#735](#735)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants