Skip to content

Coordinate validation collects once per coordinate, re-reading the index source each time #273

Description

@FBumann

Re-filed from #164, which measured the same thing against the duckdb engine and quoted SQL that no longer exists. The finding survived the rewrite in #189 almost unchanged; only the code did.

Polish rather than a fix — index sources are dim-sized — but it is N passes where one would do, and the fix folds into work that already happens.

The statement

executor.py:458 loops per coordinate, and each iteration is its own .collect():

def _check_coordinates_single_valued(self, d: str, names: list[str], frame: pl.LazyFrame) -> None:
    for c in names:
        bad = frame.group_by(d).agg(pl.col(c).n_unique().alias('n')).filter(pl.col('n') > 1).collect().height

Two of the three costs the duckdb version named carry over intact:

  1. It runs once per declared coordinate, so a dim with four coordinates makes four group_by passes over the same frame.
  2. frame is a LazyFrame over the caller's source, and for a path it is pl.scan_parquet(source) (executor.py:428). Nothing caches between iterations, so each .collect() re-opens and re-parses the file. This is the part I expected the rewrite to have fixed and it did not — laziness moved the re-read from an explicit read_parquet(...) in SQL text to an implicit one per collect().

What did go away is the third: count(DISTINCT) inside a GROUP BY was duckdb's most expensive aggregate; n_unique() in a polars agg is not special-cased the same way, so the per-pass constant is smaller than it was.

Direction

One aggregate instead of N — every coordinate in the same agg, then read which columns exceeded 1:

counts = frame.group_by(d).agg([pl.col(c).n_unique().alias(c) for c in names])
bad = counts.filter(pl.any_horizontal(pl.col(c) > 1 for c in names)).collect()

Better still, zero extra passes: _explicit_dim_frame (executor.py:419) already aggregates over the same frame to build (val, ord, coordinates…), and it is the caller of this check (executor.py:447). Folding the n_unique into that aggregate and testing the result afterwards costs nothing beyond the columns.

The error message needs the offending count and the coordinate's name, which the wide form gives directly — and would improve on today's, which reports one coordinate at a time and so names only the first of several bad ones.

Sizing note, so nobody over-invests

Bounded by dim cardinality, not by model size. On the benchmark cases this is milliseconds. It is on the list because it is a free win in a function that is already being touched, not because it shows up in a profile.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:engineLowering, IR, relational executor, sinksengine:polarsSpecific to the polars rewrite (#189) — invalidated if duckdb staysperfWall time, memory, or I/O cost — not a wrong answer

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions