Bug fix: Generate correct instances for JoinOnEntitiesNode
#1499
Conversation
This file contains 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
courtneyholcomb
force-pushed
the
court/filter-less
branch
from
1dafb8f
to
7e7a3fe
Compare
courtneyholcomb
force-pushed
the
court/filter-less
branch
from
7e7a3fe
to
9449f68
Compare
courtneyholcomb
force-pushed
the
court/filter-less
branch
from
47c3730
to
a285c63
Compare
This follows the exact same pattern as the existing group_specs_by_type() helper, but for instances.
courtneyholcomb
force-pushed
the
court/filter-less
branch
4 times, most recently
from
b1ccb3b
to
f5c1eff
Compare
courtneyholcomb
changed the title
courtneyholcomb
changed the title
JoinOnEntitiesNode
courtneyholcomb
force-pushed
the
court/filter-less
branch
from
f5c1eff
to
cd93940
Compare
courtneyholcomb
added
the
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
changed the title
JoinOnEntitiesNodeJoinOnEntitiesNode
Also general cleanup / simplification for this node's dataflow to SQL logic.
courtneyholcomb
force-pushed
the
court/filter-less
branch
from
cd93940
to
acb0295
Compare
courtneyholcomb
added
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
WilliamDee
approved these changes
Nov 4, 2024
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.
There is an internal bug in the dataflow to SQL logic for the
JoinOnEntitiesNodethat does not appear to impact production, but does impact work I'm doing up the stack. The instances generated in the node contained column associations that were not correct. They referenced the old column associations (from the parent dataset), which did not include the entity links that were added in this node. These incorrect column associations did not appear to be used for anything, and the select columns were generated from the specs instead (which is correct), so that's why this bug flew under the radar.This PR removes the logic that was causing the bug (the
AddLinkToLinkableElementsinstance converter). Instead, it adds logic to loop through the parent instances and build new instances & select columns simultaneously.This change left a lot of boilerplate & duplicate code in the function, so I moved a lot of that out to helpers for the instances and specs.
This also adds a lot of simplification for the
JoinOnEntitiesNode. It's one of our oldest nodes and was quite verbose.I recommend reviewing by commit. The snapshot changes are primarily changes to the column order and some node identifiers. There should be no actual behavior changes here.