-
Notifications
You must be signed in to change notification settings - Fork 15
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
Parity between column-level dimension links and node-level links #926
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tough one for me to review. @CircArgs any chance you could have a look please?
dimension_node, | ||
[NodeType.CUBE], | ||
) | ||
if affected_cubes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the previous line ends with or []
then this line may not be needed.
@@ -78,6 +77,9 @@ def _join_path( | |||
if col.dimension and col.dimension.type == NodeType.DIMENSION: | |||
dimensions_to_columns[col.dimension.current].append(col) | |||
|
|||
# for link in current_node.dimension_links: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgot to uncomment or not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, this is no longer needed. Removed!
…ry key mapping (if any)
Summary
This PR switches from column-level dimension links to node-level dimension links. The primary motivation for this switch is to make it so that the backend no longer needs to maintain two different storage mechanisms for dimension links -- there should just be a single storage mechanism via the
noderevision
<-->dimensionlink
tables.The changes cover the first bullet point in #927, and are primarily about reaching parity between the SQL generated via the column-level dimension links versus the node-level dimension links.
Test Plan
make check
passesmake test
shows 100% unit test coverageDeployment Plan