Skip to content

Process final query without trailing newline - #1160

Open
fallintoplace wants to merge 1 commit into
ClickHouse:mainfrom
fallintoplace:fix-benchmark-query-eof
Open

Process final query without trailing newline#1160
fallintoplace wants to merge 1 commit into
ClickHouse:mainfrom
fallintoplace:fix-benchmark-query-eof

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • preserve the final query when a query file does not end with a newline
  • apply the fix to both the regular benchmark sweep and concurrent QPS query loader

Why

Bash read fills the variable but returns a non-zero status when it reaches EOF before a newline. Since that status controls these loops, the last query was silently skipped for unterminated query files.

Testing

  • verified both newline-terminated and unterminated two-query inputs process both queries in order
  • bash -n lib/benchmark-common.sh
  • shellcheck lib/benchmark-common.sh

@alexey-milovidov

Copy link
Copy Markdown
Member

Ok, but is it important?

@fallintoplace

Copy link
Copy Markdown
Contributor Author

Yes. Two entries on main are affected: Turso and chdb-dataframe both have 43 queries but no final newline, so the current loop runs only 42.

@fallintoplace

Copy link
Copy Markdown
Contributor Author

Query 43 is a distinct workload: a minute-level time-bucket aggregation over a date range. Skipping it removes all three cold/warm measurements for that query rather than merely affecting formatting.
For chdb-dataframe, this is also a regression from the shared-runner migration on May 7. Its previous Python runner used readlines(), which preserved the unterminated final query, and the checked-in historical result files contain 43 rows with valid timings for query 43. The shared shell loop reduces future runs to 42 rows.
The same EOF behavior also affects the concurrent query loader. chdb-dataframe would benchmark only 42 queries in both the regular sweep and the concurrent workload. Turso disables the concurrent test, but its regular sweep still loses query 43.

Comment thread lib/benchmark-common.sh
# Read the same query file that bench_run_query consumed.
local queries=() q
while IFS= read -r q; do
while IFS= read -r q || [ -n "$q" ]; do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm trying to read this single line of code, and it is not obvious to me. Will it be an endless loop after consuming all results?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

read returns status 1 when it reaches EOF before a newline, but it still assigns the partial line to q. Since while A || B runs when either condition succeeds, [ -n "$q" ] is true for that final query and the body runs.

On the next condition check, the file descriptor is already at EOF. read returns 1 with no data and q becomes empty, so [ -n "$q" ] is also false. The while condition is therefore false and the loop exits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants