Skip to content

#167 feedback, but as a PR - #172

Merged
JordanMarr merged 9 commits into
JordanMarr:mainfrom
michaelglass:leftjoin-prime-followup
Sep 28, 2026
Merged

JordanMarr merged 9 commits into
JordanMarr:mainfrom
michaelglass:leftjoin-prime-followup

Conversation

@michaelglass

@michaelglass michaelglass commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

two small commits on top of #167:

1. Some col is a column everywhere, so no stripSome

this where threw on #167:

where (SqlFn.nullif (SqlFn.upper a.city, "") = Some a.addressline1)
// before (#167): NotImplementedException "Unable to evaluate query parameter expression: Some(a.addressline1)"
// after:         WHERE (NULLIF(UPPER(a.city), '') = a.addressline1)

same root cause as the on' case #167 fixed with stripSome: NProperty already treats Some col as a column, but some SQL-function arm runs NValue first, and NValue tries to evaluate Some(col). so the fix moves into NValue: it now says "not a value" for Some of a query column.

before (#167, join predicate only):

let stripSome (nexp: NormalizedExpression) =
    match nexp with
    | NMethodCall (m, [ (NColumn _) as inner ]) when m.Method.Name = "Some" -> inner
    | _ -> nexp
// ...
let left, right = stripSome left, stripSome right

after (in NValue, shared by where / having / joins):

| NMethodCall(call, [ arg ]) when call.Method.Name = "Some" && isQueryVariable arg -> None

your #167 on' tests still pass without stripSome, and the where case above is a new regression test (fails on #167, passes here).

2. one helper for the three join'/leftJoin' bodies

before, three copies of:

let innerAlias =
    match resultSelector.Parameters |> Seq.toList with
    | [_; inner] -> inner.Name
    | _ -> failwith "Expected two parameters in leftJoin result selector"
let _, innerTableMappings = TableMappings.tryGetByRootOrAlias innerAlias innerSource.TableMappings
let mergedTables = mergeTableMappings (outerSource.TableMappings, innerTableMappings)
let innerTable = mergedTables[TableAliasKey innerAlias]
let tableName = FQ.qualifiedTable innerTable
let pendingJoin = { JoinType = JoinType.Left; TableName = tableName; TableAlias = innerAlias }
let ir = mergeCtes (outerSource |> getQueryOrDefault) innerSource
this.PendingJoinInfo <- Some pendingJoin
QuerySource<'JoinResult, SelectQueryIR>(ir, mergedTables)

after:

let pending, mergedTables = pendingJoin JoinType.Left resultSelector.Parameters outerSource.TableMappings innerSource.TableMappings
this.PendingJoinInfo <- Some pending
QuerySource<'JoinResult, SelectQueryIR>(mergeCtes (outerSource |> getQueryOrDefault) innerSource, mergedTables)

also added a test that a cteFrom on the inner side of join'/leftJoin' keeps its WITH. nothing covered that before.

JordanMarr and others added 3 commits September 23, 2026 00:08
Adds a LeftJoin' overload keyed on LeftViewQuerySource so the predicate-style
join works with Schema.LeftJoined.* views, matching standard leftJoin (JordanMarr#159).
Because on' sees the post-join variable space, the joined row is already the
view, so the outer column is lifted with Some; visitJoinPredicate now strips
Some(column) so it renders as a plain column comparison.

join' over a view remains intentionally unsupported (inner joins have no null side).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ipSome

`where (SqlFn.nullif (SqlFn.upper a.city, "") = Some a.addressline1)` threw
"Unable to evaluate query parameter expression: Some(a.addressline1)".
`NProperty` already reads `Some a.addressline1` as a column, but `where` tries
its SQL-function arms first, and one of them asks `NValue` of the right side.
`NValue` compiles and evaluates any method call, and `Some(a.addressline1)`
refers to a query variable, so it cannot be evaluated. JordanMarr#167 met the same thing
in `on'` and stepped around it there with `stripSome`.

`NValue` now declines `Some` applied to a query variable's member, so a lifted
column reaches the column arms in `where`, `having` and joins alike, whatever
order they are tried in. `stripSome` goes; JordanMarr#167's `on'` tests pass without it,
and a test pins the `where` case.
The three predicate-join overloads repeated the same twenty lines: find the
inner alias, merge the table mappings, record the pending join. They differed
only in the join type and in whether the inner source's CTEs are merged, which a
left-view has none of. `pendingJoin` does the shared part.

Nothing tested that a `cteFrom` on the inner side of `join'` or `leftJoin'` keeps
its WITH clause; a test now covers both.
@michaelglass
michaelglass force-pushed the leftjoin-prime-followup branch from 2225a0f to 9cc2370 Compare September 23, 2026 14:11
@michaelglass michaelglass changed the title #167 feedback, but as a PR: Some col without stripSome, and one join helper #167 feedback, but as a PR Sep 23, 2026
JordanMarr and others added 6 commits September 28, 2026 11:27
…danMarr#168)

With no include patterns at a level, the included set was empty, so any
exclude at that level removed every item: `include = ["*"]` plus a column
exclude dropped every column (and so every table), and a table exclude with
no includes dropped every table. No includes at a level now means include all.

Filtering also rebuilt results from a Set, sorting generated record fields
(and tables) by name whenever a filter applied; results now keep source order.

Fixes JordanMarr#128

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…#170)

Materialized views had their own pipeline in `getSchema`, next to the one for
tables and views. Only the table pipeline called `SchemaFilters.filterColumns`,
so a column include or exclude that named a materialized view was ignored and
the column was still generated.

Materialized views now go through the table pipeline. The only real difference
was where their columns come from: information_schema.columns leaves them out,
so they are still read from pg_catalog. Everything after that, including
`filterColumns`, is shared. One side effect is that a materialized-view column of
an enum type now maps to the generated enum, the same as a table column does.
Before, it was dropped.
…Marr#171)

* Apply the column filters to PostgreSQL materialized views

Materialized views had their own pipeline in `getSchema`, next to the one for
tables and views. Only the table pipeline called `SchemaFilters.filterColumns`,
so a column include or exclude that named a materialized view was ignored and
the column was still generated.

Materialized views now go through the table pipeline. The only real difference
was where their columns come from: information_schema.columns leaves them out,
so they are still read from pg_catalog. Everything after that, including
`filterColumns`, is shared. One side effect is that a materialized-view column of
an enum type now maps to the generated enum, the same as a table column does.
Before, it was dropped.

* Read every relation's columns from one pg_catalog query

Materialized views took their columns from `pg_catalog` with `pg_type.typname`,
a spelling the type mappings do not use: `char(n)` columns (`bpchar`) and
domain-typed ones such as AdventureWorks' `Flag` were dropped, and the `Name`
domain bound as PostgreSQL's internal `name` type. Tables and views read
`information_schema.columns` through `GetSchema("Columns")`, which omits
materialized views, so the two paths could not simply be swapped.

One query now serves tables, views, materialized views, foreign and partitioned
tables. It spells a type the way Npgsql's `GetSchema("Columns")` does, as
`format_type` of the base type, so a domain resolves to what it wraps, and it
honours the same column privileges and positional `Columns` restrictions.
Every table and view regenerates byte-identical; the materialized views gain
their missing columns and correct types.

* Read primary keys and generated flags in the column query

The primary keys came from three joined information_schema views and the
generated/identity flags from a second pg_attribute scan, each a round trip
matched back to its column by name. Both are now fields of the one column
query: a primary key from `pg_index`, the flags from `pg_attribute` for a
table's own columns. `ColumnRef` and the two lookups go.

A primary key read from `pg_index` is visible to a role that does not own the
table, where `constraint_column_usage` hid it, so such a role now gets keys it
was missing. The generated files are otherwise byte-identical.

The pg_catalog `typname` aliases (`int8`, `bool`, `varchar`, ...) served only
the old materialized-view query; `format_type` never produces them, so they go
too. The three relation lists share one row mapper, and a restriction that is
empty matches everything, as `GetSchema` treats it.

* Pin primary-key detection with a composite key

Nothing checked which columns the Npgsql provider marks as a primary key.
person.businessentityaddress has a three-column key; every one is marked.

---------

Co-authored-by: Jordan Marr <JordanMarr@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…arr#166)

* Pin the unquoted column on the SQL-function comparison path

A column keeps its quotes when compared to a value and loses them when compared
to a SQL function:

    where (o.ORDER_DATE < System.DateTime.Now)   ->  WHERE ("o"."ORDER_DATE" < :p0)
    where (o.ORDER_DATE < SYSDATE())             ->  WHERE (o.ORDER_DATE < SYSDATE)

The value path builds a `Compare` node and the emitter runs `QuoteColumn` over
it. The function path builds a `RawWhere` whose fragment is emitted verbatim,
and `qualifyColumn` hands it the bare `alias.column`.

Oracle folds the unquoted name to upper case, looks for "O"."ORDER_DATE"
against an alias declared as "o", and fails at the server:

    ORA-00904: "O"."ORDER_DATE": invalid identifier

PostgreSQL renders it unquoted too. It survives only because it folds to lower
case and AdventureWorks is lower case throughout; a mixed-case column would
fail there as well. So this is not an Oracle bug, it is an unquoted identifier
that one dialect happens to tolerate.

Two tests, one per provider, each with the value comparison beside it as the
control. Both are `[<Ignore>]`d: they state the behaviour that is wanted, not
the behaviour there is, so removing the attribute is the acceptance criterion
for the fix.

No fix here, but the mechanism already exists and the comment says so. The
select path solves exactly this: `renderSelectExpression` marks identifiers as
`{alias}.{column}` and the emitter expands them through `QuoteRawFragment`. The
where, having and join paths do not, and `RawWhere` never calls it. Generalising
that marker to the qualified names those paths produce, which can be three parts
deep for delete and update, is the change.

* Quote the column on the SQL-function comparison path

A column kept its quotes when compared to a value and lost them when compared to
a SQL function:

    where (o.ORDER_DATE < System.DateTime.Now)   ->  WHERE ("o"."ORDER_DATE" < :p0)
    where (o.ORDER_DATE < SYSDATE())             ->  WHERE (o.ORDER_DATE < SYSDATE)

Oracle folds the unquoted name to upper case, looks for "O"."ORDER_DATE" against
an alias declared as "o", and rejects the statement with ORA-00904. PostgreSQL
rendered it unquoted too and only survived because it folds to lower case and
AdventureWorks is lower case throughout; a mixed-case column would have failed
there as well.

The select path had already solved this. `renderSelectExpression` and the other
raw-fragment producers mark a column as `{alias}.{column}`, and the emitter
expands the marker into properly quoted identifiers via `QuoteRawFragment` on the
select, orderBy, groupBy and setRaw paths. The where, having and join paths never
adopted the marker, and `RawWhere` never called the expander, so a column that
landed inside one of their fragments reached the server bare.

So the fix keeps the existing marker convention rather than inventing a second
one:

  - `markQualified` wraps an already-qualified dotted name in per-segment braces,
    and `markingQualifier` lifts a builder's `qualifyColumn` to produce marked
    names. `visitWhere`, `visitHaving` and `visitJoinPredicate` now pass that
    lifted qualifier — and only that one — to everything that renders into a raw
    fragment. A column bound for a structured clause (`Compare`, `IsNull`,
    `CompareColumns`) still gets the bare name, because the emitter quotes it
    there. The five places that previously spelled the marker inline now call
    `markQualified`, so the convention lives in one function.

  - `RawWhere` emission runs `QuoteRawFragment`, like the other raw paths.

  - `QuoteRawFragment` matched a two-part `{a}.{b}` only. Select qualifies two
    parts deep, delete and update three (`{schema}.{table}.{column}`), so the
    pattern now matches a run of brace segments joined by dots and quotes each.
    Requiring at least two segments keeps a lone `{...}` in a hand-written
    fragment untouched, which matters because `havingRaw` fragments now flow
    through the expander as `setRaw` and `rawExpr` fragments always have.

The two regression tests lose their `[<Ignore>]`, a new Oracle integration test
runs the previously-failing query against the server, and two more cover the
three-part delete and update qualification. Twenty-one Npgsql assertions that
pinned the unquoted rendering are updated to the quoted SQL.

* Mark columns in one helper, and quote the orderBy function path by dialect

The method-call `orderBy` path and `lateralCol` still wrote `"alias"."col"`
by hand, which is PostgreSQL's quoting and wrong on SQL Server and MySQL. Both
now mark the column and let the emitter quote it like every other raw fragment.

`markColumn alias mem` replaces the `markQualified $"{alias}.{name}"` spelled
out at six sites. `nVisitSqlFn` marks the columns it renders itself, so its ten
callers pass the plain qualifier and cannot forget. `QuoteRawFragment` reuses
`QuoteDotted`, builds its regex once, and skips a fragment with no `{`. The
tests say what they pin; the story of the bug stays in the history.
The lifted-column test avoided asserting quotes, but its "addressline1)"
check assumed the column stayed unquoted. With JordanMarr#166 on main it renders as
"a"."addressline1", so assert the whole fragment.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@JordanMarr
JordanMarr merged commit f58641f into JordanMarr:main Sep 28, 2026
1 check passed
@michaelglass
michaelglass deleted the leftjoin-prime-followup branch September 28, 2026 17:34
@JordanMarr JordanMarr mentioned this pull request Sep 28, 2026
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.

2 participants