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

Refactor block_using_rules_sql to follow normal pattern and avoid confusion #1695

Merged
merged 3 commits into from
Nov 3, 2023

Conversation

RobinL
Copy link
Member

@RobinL RobinL commented Nov 3, 2023

Type of PR

  • BUG
  • FEAT
  • MAINT
  • DOC

Is your Pull Request linked to an existing Issue or Pull Request?

This pr revealed a mistake I made in blocking.block_using_rules_sql whereby the function was not a pure function that returned just SQL.

Instead, it the case of a _two_dataset_link_only it was running _enqueue_sql - a side effect that means this sql gets 'lost' if you look at the results of the function.

This is contrary to the pattern elsewhere in the codebase where, if multiple tables need queuing, then the function should return a list of sql statements. This pattern is used everywhere we see code like:

for sql in sqls:
        linker._enqueue_sql(sql["sql"], sql["output_table_name"])

Give a brief description for the solution you have provided

I've adjusted block_using_rules_sql to be block_using_rules_sqls.

Think it's a good idea to get this merged before #1664 to try and keep additional complexity to a minimum

@RobinL RobinL requested a review from ADBond November 3, 2023 10:36
Copy link
Contributor

@ADBond ADBond left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@ADBond
Copy link
Contributor

ADBond commented Nov 3, 2023

I wonder if maybe the fact that we use

for sql in sqls:
        linker._enqueue_sql(sql["sql"], sql["output_table_name"])

so many places means we could probably do with a shortcut helper, but that is probably best left as a separate one-and-done task across the codebase. Will open an issue

@RobinL
Copy link
Member Author

RobinL commented Nov 3, 2023

Thanks!

Yeah agreed, would make sense to have a _enqueue_multiple_sqls

@RobinL RobinL merged commit a0240cf into master Nov 3, 2023
8 checks passed
@RobinL RobinL deleted the blocking_sql_queueing_refactor branch November 3, 2023 13:34
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