Support blocking sections with multiple exploded columns #143
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
Instead of trying to select() out all of the columns we need in each iteration, we can use withColumn() to add the exploded columns one by one. There is a weird bit of logic where we need to do an extra select only if there are exploded columns. I added a comment about that and will do a bit more looking into it.
…ting them out Previously we were selecting with a set, so the columns got all mixed up. Let's sort them so that they are easier to work with. The order of the columns should not affect the results.
Check the size of the output tables to confirm that rows are being exploded correctly.
anpumn
approved these changes
Aug 14, 2024
riley-harper
commented
Aug 14, 2024
| ) | ||
| for c in all_column_names | ||
| ] | ||
| explode_col_expr = explode(col(exploding_column_name)) |
There was a problem hiding this comment.
Why is this exploding_column_name and not derived_from_column?
ccdavis
approved these changes
Aug 14, 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.
Fixes #142.
This PR fixes a bug in the matching explode step which seems like it has been around for a long time. When a user set
explode = truefor more than one blocking column, they got an error like this:This error happened in the loop in the
_explode()function. The loop constructed each exploded column one-by-one and then selected it and the other columns out ofexploded_df. The problem was that each iteration tried to select all of the blocking columns out ofexploded_df, even though the loop hadn't run for all of the exploded columns yet. This is why the first iteration of the loop threw an unresolved column error.To fix this, I've switched the loop to add the exploded columns to
exploded_dfone-by-one withwithColumn(). This actually ended up simplifying the loop pretty substantially because we can focus on a single column at a time instead of handling all of the columns to select out with list comprehensions. The results are the same.I found another possible bug when working on this. In the previous implementation, we selected
all_column_namesout ofexploded_dfif and only if there was at least one exploded column. The tests depend on this behavior. So I've added some logic to replicate the behavior and a comment explaining it. Changing this behavior is probably technically a breaking change because it changes the columns of the exploded_df_a and exploded_df_b tables. This might have ramifications for later tasks as well. One thing I did change is the order of the columns in exploded_df_a and exploded_df_b, since previously we were selecting with an unordered set. I've sorted the columns so that they aren't in a random order in the output tables.