Skip to content

fix(core): order grouped table rows before applying limit - #3336

Open
dvd233 wants to merge 1 commit into
evidence-dev:mainfrom
dvd233:fix/table-limit-after-sort
Open

dvd233 wants to merge 1 commit into
evidence-dev:mainfrom
dvd233:fix/table-limit-after-sort

Conversation

@dvd233

@dvd233 dvd233 commented Sep 13, 2026

Copy link
Copy Markdown

Summary

  • infer SQL ordering from a non-pivot table column's sort metadata when limit is set
  • quote the inferred alias for each SQL dialect while preserving an explicit table-level order
  • keep pivoted, comparison, and sparkline render-column sorting on the client
  • add regression coverage proving the largest groups are selected before pagination

Problem

Table limit is applied in SQL, while declarative column sorting normally happens after the query. When a grouped result has more than limit rows, the warehouse can therefore truncate an arbitrary subset before the client sorts it, silently excluding the actual top rows.

This addresses the top-N ordering part of #3334. It intentionally does not change the existing subtotal suppression under limit or the separate null-dimension rendering behavior described in that issue.

Testing

  • regression test fails against the current main implementation and passes with this change
  • node_modules/.bin/svelte-check --threshold error --tsconfig core/tsconfig.json — 0 errors
  • focused Table + shared SQL/dialect Vitest suites — 468 passed
  • generated-order probe across ClickHouse, Snowflake, BigQuery, Fabric, Databricks, PostgreSQL, Cube, and MotherDuck

The npm chdb package does not provide its native binary on Windows, so the local Vitest run used a temporary, uncommitted transport adapter to execute the existing ClickHouse assertions against play.clickhouse.com. The committed test and test utilities are unchanged in that respect; normal Linux CI will continue to use the repository's native chdb path.

@vercel

vercel Bot commented Sep 13, 2026

Copy link
Copy Markdown

@dvd233 is attempting to deploy a commit to the Evidence Team on Vercel.

A member of the Team first needs to authorize it.

@tojacob03 tojacob03 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I ran this on macOS (arm64) with the native chdb build. The description says the ClickHouse assertions only ran through a temporary adapter on Windows, so I wanted to check them against the real one.

  • Rebased onto current main (c2dd7c2): the cherry-pick applies cleanly.
  • table.test.ts: 24/24 pass, including the 4 new tests.
  • With build-table-sql.ts reverted to main, "applies a column sort before limiting grouped rows" fails, so the regression test does catch the bug.
  • Full core suite: 3990 passed, 3 skipped.

Two questions:

1. Which column wins when several have sort?
For the initial client-side sort, Table.svelte uses the first column in columnMeta that has sort. This PR pushes down the first column with sort that can be pushed down. The two differ when the first sorted column is excluded (sparkline, derived, pivot) and a later one isn't. For example:

cols(dim('category'), measure('sum(total_sales)'), measure('count(*)'))
// [1]: sort = 'desc', viz = 'sparkline'
// [2]: sort = 'asc'
// limit: 5

This gives order = '"count" asc', so the top 5 rows are picked by count but displayed sorted by the sparkline column. Would it be safer to push the sort down only when the column the client sorts by can itself be pushed down, and leave order undefined otherwise?

2. Paging together with limit (probably out of scope)
With limit + page_size/offset, the generated SQL is:

SELECT * FROM (... ORDER BY "sum_total_sales" desc LIMIT 5) AS evidence_paged LIMIT 2 OFFSET 2

The outer query has no ORDER BY, and most engines don't promise to keep a subquery's order, so pages could overlap or skip rows. This comes from the existing wrapper in sql-options.ts (it happens with an explicit order too), so it isn't caused by this PR. I'm only mentioning it in case it's worth a follow-up.

@hughess

hughess commented Sep 29, 2026

Copy link
Copy Markdown
Member

/upstream

@github-actions

Copy link
Copy Markdown
Contributor

Imported for internal review, this PR will be updated when it merges.

This branch has not been deployed

No deployments
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.

3 participants