Merged
Conversation
Merged
5 tasks
|
✅ 35/35 passed, 7 flaky, 2 skipped, 25m25s total Flaky tests:
Running from acceptance #301 |
nfx
requested changes
Jul 10, 2024
| def replace_database_in_query(node: sqlglot.Expression, *, database: str) -> sqlglot.Expression: | ||
| """Replace the database in a query.""" | ||
| if isinstance(node, sqlglot.exp.Table) and node.args.get("db") is not None: | ||
| def replace_database_in_query( |
Collaborator
There was a problem hiding this comment.
rewrite this as a public method on DashboardMetadata: def replace_database(self, catalog_name: str, schema_name: str) -> DashboardMetadata:, which would create a copy of DashboardMetadata, but with all of the queries rewritten with the target catalog/db
nfx
requested changes
Jul 10, 2024
0b51df7 to
7a7366f
Compare
bbc3cb6 to
cba91fe
Compare
c2b2083 to
1ff8635
Compare
1ff8635 to
cc4ce27
Compare
JCZuurmond
commented
Jul 11, 2024
|
|
||
| def update(self, other: "TileMetadata") -> None: | ||
| """Update the tile metadata with another tile metadata. | ||
| def merge(self, other: "TileMetadata") -> "TileMetadata": |
Contributor
Author
There was a problem hiding this comment.
@nfx : Refactored this method also to an implementation that is stateless
nfx
requested changes
Jul 11, 2024
| replace_database_in_query = functools.partial(dashboards.replace_database_in_query, database=database) | ||
| lakeview_dashboard = lakeview_dashboards.create_dashboard(folder, query_transformer=replace_database_in_query) | ||
| dashboard_metadata = DashboardMetadata.from_path(folder).replace_database( | ||
| catalog=catalog or None, database=database or None |
Collaborator
There was a problem hiding this comment.
Suggested change
| catalog=catalog or None, database=database or None | |
| catalog=catalog or None, database=database or None, |
nit
|
|
||
| def update(self, other: "TileMetadata") -> None: | ||
| """Update the tile metadata with another tile metadata. | ||
| def merge(self, other: "TileMetadata") -> "TileMetadata": |
| query_transformer: Callable[[sqlglot.Expression], sqlglot.Expression] | None = None, | ||
| ) -> Dashboard: | ||
| """Create a dashboard from code, i.e. configuration and queries. | ||
| def create_dashboard(dashboard_metadata: DashboardMetadata) -> Dashboard: |
Collaborator
There was a problem hiding this comment.
it looks like the entire create_dashboard method from here should be converted to a method on DashboardMetadata:
def as_lakeview(self) -> Dashboard:
self.validate()
datasets = self.get_datasets() # make get_datasets private
layouts = self.get_layouts() # make get_layouts private
page = Page(
name=dashboard_metadata.display_name,
display_name=dashboard_metadata.display_name,
layout=layouts,
)
return Dashboard(datasets=datasets, pages=[page])
This was referenced Jul 11, 2024
nfx
pushed a commit
that referenced
this pull request
Jul 11, 2024
…ines (#214) Implementing feedback: #210 (comment)
nfx
added a commit
that referenced
this pull request
Jul 11, 2024
* Added method to dashboards to get dashboard url ([#211](#211)). In this release, we have added a new method `get_url` to the `lakeview_dashboards` object in the `laksedashboard` library. This method utilizes the Databricks SDK to retrieve the dashboard URL, simplifying the code and making it more maintainable. Previously, the dashboard URL was constructed by concatenating the host and dashboard ID, but this new method ensures that the URL is obtained correctly, even if the format changes in the future. Additionally, a new unit test has been added for a method that gets the dashboard URL using the workspace client. This new functionality allows users to easily retrieve the URL for a dashboard using its ID and the workspace client. * Extend replace database in query ([#210](#210)). This commit extends the database replacement functionality in the `DashboardMetadata` class, allowing users to specify which database and catalog to replace. The enhancement includes support for catalog replacement and a new `replace_database` method in the `DashboardMetadata` class, which replaces the catalog and/or database in the query based on provided parameters. These changes enhance the flexibility and customization of the database replacement feature in queries, making it easier for users to control how their data is displayed in the dashboard. The `create_dashboard` function has also been updated to use the new method for replacing the database and catalog. Additionally, the `TileMetadata` update method has been replaced with a new merge method, and the `QueryTile` and `Tile` classes have new properties and methods for handling content, width, height, and position. The commit also includes several unit tests to ensure the new functionality works as expected. * Improve object oriented dashboard-as-code implementation ([#208](#208)). In this release, the object-oriented implementation of the dashboard-as-code feature has been significantly improved, addressing previous pull request comments ([#201](#201)). The `TileMetadata` dataclass now includes methods for updating and comparing tile metadata, and the `DashboardMetadata` class has been removed and its functionality incorporated into the `Dashboards` class. The `Dashboards` class now generates tiles, datasets, and layouts for dashboards using the provided `query_transformer`. The code's readability and maintainability have been further enhanced by replacing the use of the `copy` module with `dataclasses.replace` for creating object copies. Additionally, updates have been made to the unit tests for dashboard functionality in the project, with new methods and attributes added to check for valid dashboard metadata and handle duplicate query or widget IDs, as well as to specify the order in which tiles and widgets should be displayed in the dashboard.
Merged
nfx
added a commit
that referenced
this pull request
Jul 11, 2024
* Added method to dashboards to get dashboard url ([#211](#211)). In this release, we have added a new method `get_url` to the `lakeview_dashboards` object in the `laksedashboard` library. This method utilizes the Databricks SDK to retrieve the dashboard URL, simplifying the code and making it more maintainable. Previously, the dashboard URL was constructed by concatenating the host and dashboard ID, but this new method ensures that the URL is obtained correctly, even if the format changes in the future. Additionally, a new unit test has been added for a method that gets the dashboard URL using the workspace client. This new functionality allows users to easily retrieve the URL for a dashboard using its ID and the workspace client. * Extend replace database in query ([#210](#210)). This commit extends the database replacement functionality in the `DashboardMetadata` class, allowing users to specify which database and catalog to replace. The enhancement includes support for catalog replacement and a new `replace_database` method in the `DashboardMetadata` class, which replaces the catalog and/or database in the query based on provided parameters. These changes enhance the flexibility and customization of the database replacement feature in queries, making it easier for users to control how their data is displayed in the dashboard. The `create_dashboard` function has also been updated to use the new method for replacing the database and catalog. Additionally, the `TileMetadata` update method has been replaced with a new merge method, and the `QueryTile` and `Tile` classes have new properties and methods for handling content, width, height, and position. The commit also includes several unit tests to ensure the new functionality works as expected. * Improve object oriented dashboard-as-code implementation ([#208](#208)). In this release, the object-oriented implementation of the dashboard-as-code feature has been significantly improved, addressing previous pull request comments ([#201](#201)). The `TileMetadata` dataclass now includes methods for updating and comparing tile metadata, and the `DashboardMetadata` class has been removed and its functionality incorporated into the `Dashboards` class. The `Dashboards` class now generates tiles, datasets, and layouts for dashboards using the provided `query_transformer`. The code's readability and maintainability have been further enhanced by replacing the use of the `copy` module with `dataclasses.replace` for creating object copies. Additionally, updates have been made to the unit tests for dashboard functionality in the project, with new methods and attributes added to check for valid dashboard metadata and handle duplicate query or widget IDs, as well as to specify the order in which tiles and widgets should be displayed in the dashboard.
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.
Improve the database replacement in query by:
DashboardMetadataclassSee https://github.com/databrickslabs/ucx/pull/1920/files/fae429e4d73eceebd7259c9bcaa390e6c7aa4395#r1670960522
Partially resolves #212