-
Notifications
You must be signed in to change notification settings - Fork 680
Stream input from stdin, rather than reading the entire script into memory. #1450
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
base: main
Are you sure you want to change the base?
Conversation
283fa03 to
7bae606
Compare
The one breaking change here is that previously, the header lines would have been printed for every query in the script, leading to every other line being a header. It seems improbable that anyone would be depending on that behavior. Edit: this description of the previous behavior may not be correct, and needs checking. Edit: the description of the previous behavior was correct. |
|
What's the intended use case of only showing the header for the first query? That seems to be more confusing at first glance, as you lose the details of what is actually happening. I.e.: test.sql queries: command Current behavior (able to see the header for each, so you know what the values are for: New behavior: Makes it look like all the values are count(*)s due to only the one header. |
The intended use case is a uniform script which produces the same columns for each line. I think that's the most common usecase when scripting! But maybe we can make it more configurable. |
7bae606 to
69729cf
Compare
Hmm I'd have to see some examples of what you mean. Anything I can think of I'd want to preserve the headers for each query. I.e. for a migration script, there might be various selects to check different things, some updates/inserts, etc. where I'd expect each query to come back with whatever headers it needed. If you're actually doing something with the output, i.e. creating a CSV, it's going to be in one query, and only have one header then regarless. |
As a data practitioner, this pattern is common. But again, we can finesse it with the CLI args. |
69729cf to
b6d6143
Compare
rather than reading the entire script into memory. * Stream STDIN input, running queries a line at a time. * Remove MemoryError check, and recommendation for the vendor client. * Use CSV/TSV formats with headers for the first line only. * Exit with an error code if we are unable to open /dev/tty. * Add --noninteractive flag to suppress the destructive-warning prompt from the CLI. * Commentary on edge cases and followups.
b6d6143 to
4253bad
Compare
Huh interesting. I'd expect to do that in a loop; putting that all in a SQL file (and running as a combo of queries at once) seems like an anti-pattern. |
Also the vendor client works as mycli currently does, with headers for each query. I know we are expanding on the vendor client, but in this case I prefer the current behavior that matches with headers per query. Edited to add; if you are planning to add an option to control this, I would say have the current behavior be the default at least. |
|
My main issue is with the specification of |
Description
MemoryErrorcheck, and recommendation for the vendor client./dev/tty.--noninteractiveflag to suppress the destructive-warning prompt from the CLI.Fixes #1184.
Checklist
changelog.md.AUTHORSfile (or it's already there).uv run ruff check && uv run ruff format && uv run mypy --install-types .to lint and format the code.