Skip to content

Normalize rowLimit across connection backends - #2980

Draft
MianliWang wants to merge 1 commit into
malloydata:mainfrom
MianliWang:fix-2786-normalize-row-limit
Draft

Normalize rowLimit across connection backends#2980
MianliWang wants to merge 1 commit into
malloydata:mainfrom
MianliWang:fix-2786-normalize-row-limit

Conversation

@MianliWang

Copy link
Copy Markdown
Contributor

Summary

  • add shared rowLimit configuration metadata and validation with a default of 1000
  • thread malloy.config row limits through every registered database connection factory
  • apply consistent per-run > connection config > default precedence in batch and streaming paths
  • add regression coverage for zero-row limits, per-run overrides, streaming, and the former 10-row cap

Fixes #2786

Root cause

rowLimit existed as a run option, but connection factories did not consistently register or forward the same setting from malloy.config. Individual backends then supplied independent defaults, including several hard-coded limits of 10. As a result, configuration could validate incompletely, be dropped during connection construction, or behave differently between batch and streaming execution.

Resulting behavior

  • precedence is per-run rowLimit > connection rowLimit > 1000
  • rowLimit: 0 is valid and returns no rows
  • invalid values (negative, fractional, non-numeric, NaN, or infinite) are rejected
  • Publisher forwards an explicit local override, but when no local value is supplied it leaves the remote server's configured default intact

Areas that need focused review

  1. Shared policy and public surface
    • packages/malloy/src/connection/query_options.ts
    • whether 1000 is the desired common default and whether zero should remain valid
  2. Factory configuration wiring
    • BigQuery, Databricks, DuckDB native, DuckDB WASM, MySQL, Postgres, Snowflake, Trino/Presto, and Publisher registrations in their respective src/index.ts, native.ts, or browser.ts files
    • each factory must register rowLimit and pass the resolved connection value without replacing a valid zero
  3. Backend-specific execution behavior
    • Publisher: omitted local configuration must not override the remote default
    • Databricks: bounded single-chunk fetches and totalRows reporting fetched rows rather than draining the operation
    • Trino: cancellation after reaching the bound; Presto: schema EXPLAIN uses a one-row internal override without wrapping user SQL
    • BigQuery, DuckDB, Postgres, and Snowflake: streaming stops exactly at the bound, including zero
  4. Configuration/API compatibility
    • generated malloy-config.json schema exposure and validation behavior
    • callers that previously depended on backend-specific defaults of 10

Validation

  • relevant TypeScript package build passed for Malloy plus BigQuery, Databricks, DuckDB, MySQL, Postgres, Publisher, Snowflake, and Trino
  • ESLint passed for all changed TypeScript files
  • 10 focused suites passed: 162 tests passed, 10 skipped
  • focused DuckDB configuration tests passed, including the issue reproduction with limit: 25
  • DuckDB zero-row streaming regression passed
  • git diff --check passed

The full DuckDB suite also reaches two unrelated Windows idle file-lock failures; the focused row-limit and zero-streaming tests pass. A companion documentation update in malloydata/malloydata.github.io is still needed.

Signed-off-by: Rough-Egoist <mianli.wang@mail.utoronto.ca>
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.

rowLimit normalization needed across connection backends

1 participant