-
Notifications
You must be signed in to change notification settings - Fork 124
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
chore: cleanup tpch execute #1374
Conversation
CUSTOMER_PATH = DATA_DIR / "customer.parquet" | ||
|
||
BACKEND_READ_FUNC_MAP = { | ||
# "pandas": lambda x: pd.read_parquet(x, engine="pyarrow"), # noqa: ERA001 |
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.
should we just remove these? I kept them because they were still in the dictionary
result.returncode == 0 | ||
), f"Script {script_path} failed with error: {result.stderr}" | ||
|
||
@pytest.mark.parametrize("query_path", QUERIES_DIR.glob("q[1-9]*.py")) |
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 this PR doesn't get merged, I think that at least we should have this parametrised :)
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.
+1
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.
I love this kind of refactors and simplifications! Thanks @EdAbati !
I left just one comment regarding enabling running dask since they fixed the issue, and from our side we adjusted given their suggestions
result.returncode == 0 | ||
), f"Script {script_path} failed with error: {result.stderr}" | ||
|
||
@pytest.mark.parametrize("query_path", QUERIES_DIR.glob("q[1-9]*.py")) |
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.
+1
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.
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.
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below.
Inspired by #1348, I thought it would be handy if adding/removing backends from TPCH was a bit easier. With this change, adding/removing a key in the
BACKEND_READ_FUNC_MAP
dictionary would add/remove a backend from the test.While refactoring this I found some duplicated (a path was present twice) and unused code (the read functions that I commented out). I hope that with this it would be easier to spot
The diff is big because I removed all the
execute
folder. I know that this is lower priority so don't feel like you have to review straight away, it can wait :)