exact_rows_before_limit wrong for numbers/primes/generate_series with LIMIT

Open
#121,195 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
72/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
cpp, sql
Domain
databases

Research direction

Run the provided numbers(300) LIMIT 5 SQL reproducer with exact_rows_before_limit = 1, then inspect shouldPushdownLimit in src/Processors/QueryPlan/numbersLikeUtils.cpp and the limit handling in src/Processors/QueryPlan/Optimizations/optimizePrimaryKeyConditionAndLimit.cpp. Compare the behavior with limitPushDown.cpp and a MergeTree query. Done means numbers, numbers_mt, generate_series, and primes report the full pre-LIMIT count while still returning the limited rows.

Written by the indexing model from the issue text.

Description

comp-query-optimizer
Describe what's wrong
  • rows_before_limit_at_least equals the LIMIT value instead of the true source row count.
  • Triggered by SELECT number FROM numbers(300) LIMIT 5 with setting exact_rows_before_limit = 1.
  • Expected rows_before_limit_at_least: 300, actual returns 5.
  • MergeTree tables report the correct value for the same query shape, so results disagree by source type.

Root cause: shouldPushdownLimit in numbersLikeUtils.cpp has no check for exact_rows_before_limit/alwaysReadTillEnd, so ReadFromSystemNumbersStep caps generation at the LIMIT and the counter never sees the remaining rows; optimizePrimaryKeyConditionAndLimit.cpp has the same missing guard that limitPushDown.cpp already applies.

Analysis details (evidence, affected locations, impact)

Why we believe this is a bug: ReadFromSystemNumbersStep ctor (ReadFromSystemNumbersStep.cpp:431) -> NumbersLikeUtils::getLimitFromQueryInfo (numbersLikeUtils.cpp:133) -> shouldPushdownLimit (numbersLikeUtils.cpp:70-74) returns true -> the step's limit member is set to limit_length + limit_offset -> makePipe shrinks the generated range to it (ReadFromSystemNumbersStep.cpp:660-662) -> the source emits 5 rows, so the counter feeding rows_before_limit_at_least never sees the other 295.

Affected locations:

Impact: With exact_rows_before_limit = 1 -- the setting whose only purpose is to make this statistic exact, documented as 'the data before limit will have to be read completely' (Settings.cpp:3798) -- a LIMIT over numbers/numbers_mt/generate_series/primes returns a rows_before_limit_at_least equal to the LIMIT instead of the true pre-LIMIT count. Clients that use the field for a total-row count (UIs, pagination) get a wrong total with no error. A MergeTree table under the identical query reports the correct count, so the two disagree.

Does it reproduce on most recent release?

Yes — confirmed on current master (commit 827b4f0e149c6).

How to reproduce

Run with SETTINGS exact_rows_before_limit = 1 and compare against the same query over a MergeTree table or a UNION ALL of numbers.

▶ Run on ClickHouse Fiddle

-- Test: exact_rows_before_limit must report the full pre-LIMIT row count for numbers().

SELECT number FROM numbers(300) LIMIT 5
FORMAT JSONCompact
SETTINGS exact_rows_before_limit = 1, output_format_write_statistics = 0;

Note: an automated re-run of this exact block on current master (827b4f0e149c) did not show the failure; the analyst's run did (outputs below). Environment or ordering may matter.

Expected behavior

With exact_rows_before_limit set, rows_before_limit_at_least should reflect the full pre-LIMIT row count, reading all source rows if needed.

Expected output of the reproducer above:

{
	"meta":
	[
		{
			"name": "number",
			"type": "UInt64"
		}
	],

	"data":
	[
		[0],
		[1],
		[2],
		[3],
		[4]
	],

	"rows": 5,

	"rows_before_limit_at_least": 300
}
Error message and/or stacktrace

The LIMIT is pushed into the source generator, which stops early, so rows_before_limit_at_least equals the LIMIT.

Actual output of the reproducer above on master (827b4f0e149c6):

{
	"meta":
	[
		{
			"name": "number",
			"type": "UInt64"
		}
	],

	"data":
	[
		[0],
		[1],
		[2],
		[3],
		[4]
	],

	"rows": 5,

	"rows_before_limit_at_least": 5
}
Suggested fix

Add an exact_rows_before_limit rejection to shouldPushdownLimit (it already receives query_info, and its caller getLimitFromQueryInfo holds the ContextPtr), and guard optimizePrimaryKeyConditionAndLimit.cpp:65 with !limit_step->alwaysReadTillEnd() exactly as limitPushDown.cpp:67 does. Trade-off: with the setting on, SELECT ... FROM numbers(N) LIMIT k reads all N rows instead of k -- which is the documented cost of the setting, and what MergeTree already does.

Additional context

Same pattern as #103445 (found by: lexical, vector; vector: cosine distance 0.15 (agrees with the lexical leg)).

Open risks:

  • always_read_till_end is also set by WITH TOTALS (InterpreterSelectQuery.cpp limitAlwaysReadsTillEnd), but those shapes put an aggregation between the LIMIT and the source, so the walk in optimizePrimaryKeyConditionAndLimit breaks before setLimit and shouldPushdownLimit rejects on need_aggregate. I could not construct a WITH TOTALS repro; exact_rows_before_limit is the only trigger I could reach.

Found during automated review of PR #118131; whether that PR introduced it could not be established, so nobody is tagged. Severity P2 · Finding h_pr118131_001

Dominant language
C++
Stars
50k
Forks
9k
Avg merge
18h 29m
Merged PRs (30d)
511

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from ClickHouse/ClickHouse

All issues in ClickHouse/ClickHouse

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.