advanced lazy sqlpage functions#1348
Conversation
SQLPage functions that are the whole value of a selected column now execute after the database query, once per returned row. If the query returns no rows, the function is not called. This ensures deterministic behavior and prevents expensive or side-effectful operations from running unnecessarily.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab77de705
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(error) = apply_delayed_functions(request, &statement.delayed_functions, &mut row) | ||
| .instrument(query_span.clone()) | ||
| .await |
There was a problem hiding this comment.
Here the scalar SET path reuses apply_delayed_functions, but that helper always starts with let mut db_conn = None, so a delayed sqlpage.run_sql in SET x = sqlpage.run_sql(...) runs its included file on a fresh pooled connection even though the scalar query stream has already been dropped. Pages that create temp tables or start a transaction before the SET now lose that session state (the old pre-query FunctionCall path used the request db_connection), and the docs recommend SET for one-shot run_sql; this path should pass the existing connection or avoid delaying SET functions that need it.
Useful? React with 👍 / 👎.
|
Summary
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a15e18951
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| argument_col_names.push(argument_col_name.clone()); | ||
| replacement_items.push(SelectItem::ExprWithAlias { | ||
| expr: take_function_arg_expr(arg), | ||
| alias: Ident::with_quote('"', argument_col_name), |
There was a problem hiding this comment.
When a delayed SQLPage function is aliased and that alias is referenced elsewhere in the same SELECT, this rewrite removes the alias from the SQL sent to the database without updating those clauses. For example, SELECT sqlpage.url_encode(url) AS encoded FROM t ORDER BY encoded is rewritten to SELECT url AS "_sqlpage_f0_a0" FROM t ORDER BY encoded, so the database now rejects a query in the newly supported “function over a column” shape because encoded no longer exists during query execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f030aa1b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| .await); | ||
| } | ||
| scalar_value_from_row(row)? |
There was a problem hiding this comment.
When scalar_value_from_row rejects the row, for example with SET x = (SELECT 1 AS a, 2 AS b), this ? returns before record_error_and_rollback, unlike the DB, multiple-row, and delayed-function error paths above. If the page has already run BEGIN on the request connection, this newly introduced scalar-shape error leaves that transaction open on the pooled connection and records no query error; route this error through the same rollback path before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ec26700ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(func_name) = delayable_sqlpage_function(&function) | ||
| && (!is_constant_single_row || !preserves_null_concat_semantics(&function)) |
There was a problem hiding this comment.
When a standalone SQLPage call has no row-dependent arguments, this condition only preserves the old pre-query evaluation for concat; other argument helpers that SQLPage used to emulate, such as json_build_object/json_array/coalesce, are now emitted as hidden SQL for the database to execute. On SQLite, a previously valid SELECT sqlpage.url_encode(json_build_object('k', :v)) AS x now becomes a query that calls json_build_object(...) in SQLite, which has no such function, so the page fails before url_encode runs. If the arguments can be converted with function_args_to_stmt_params and do not depend on a returned row, they should stay on the pre-query path rather than only special-casing concat.
Useful? React with 👍 / 👎.
Replace the `StmtParam`/`ParameterExtractor` machinery with a shared `SqlPageExpr` type parameterized over its input source, distinguishing standalone expressions from per-row expressions via the type system. Rename `ParsedSqlFile` to `SqlFile` and split SQL parsing into dedicated `dialect`, `rewrite`, and `statement` submodules. Queries are now rewritten into either a database query with bindings and computed columns, or a single-row query evaluated without a database round trip. Make `SqlPageFunctionName` parsing case-insensitive and support SQLPage-computed projections and nested functions over query results.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e07594579b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| item => database_projection.push(item), | ||
| } | ||
| } | ||
| database_projection.append(&mut rewriter.private_projection); |
There was a problem hiding this comment.
On MySQL/ODBC positional placeholders, appending the private row-input projections moves their ? placeholders after any later public columns, while finish_bindings still sorts bindings by original source span. For example, SELECT $a AS a, sqlpage.url_encode(col || $b) AS b, $c AS c FROM t renders placeholders in $a,$c,$b order but binds $a,$b,$c, so both the public column and the computed function receive the wrong request values. Keep private projections in their original projection position or order bindings by the rendered SQL.
Useful? React with 👍 / 👎.
| if let Some(order_by) = &query.order_by | ||
| && let OrderByKind::Expressions(expressions) = &order_by.kind | ||
| && expressions.iter().any(|ordering| { |
There was a problem hiding this comment.
When ORDER BY uses column ordinals, this guard only checks identifier aliases and misses the case where removing computed projections changes the database select-list positions. For example, SELECT a, sqlpage.url_encode(b) AS enc, c FROM t ORDER BY 3 now sends the database a select list ordered as a,c,<private b>, so ORDER BY 3 sorts by the private b input instead of the user's c column. Preserve projection positions or reject ordinal ordering whenever SQLPage-computed columns are present.
Useful? React with 👍 / 👎.
Cover cases where SQLPage functions span database and per-row boundaries, including predicates, projections, and shared request/row values. Also add documentation to `QueryResult` and `BoundQuery`, tidy up borrow expressions in the OIDC client, and rename a conflicting `rows` alias in a test fixture.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 947b189020
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(error) = evaluate_computed_columns( | ||
| request, | ||
| &database_query.computed_columns, | ||
| &mut row, | ||
| db_connection, | ||
| ) |
There was a problem hiding this comment.
When a scalar SET row contains both a normal column and a SQLPage-computed column, e.g. SET x = (SELECT 1 AS a, sqlpage.fetch(url) AS b FROM cache_misses LIMIT 1), this evaluates fetch/exec/run_sql before scalar_value_from_row rejects the two-column result. That makes an invalid scalar assignment perform external side effects; the final column count should be rejected before evaluating computed columns that are not needed.
Useful? React with 👍 / 👎.
| if recognize_sqlpage_function(function)?.is_none() | ||
| && emulated_function(function).is_none() |
There was a problem hiding this comment.
Allowing emulated functions in the single-row fast path means ordinary database expressions such as SELECT concat('Hello ', $name) AS title are no longer executed by the configured DB. On PostgreSQL, concat('Hello ', NULL) returns Hello , while SQLPage's Concat returns NULL when any argument is NULL, so pages with optional variables can render different values after this change. Restrict this fast path to SQLPage-owned expressions, or leave plain database functions to the DB.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c45103eb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let first_ordinal = database_projection.len() - (rewriter.row_inputs.len() - first_input); | ||
| for (offset, input) in rewriter.row_inputs[first_input..].iter_mut().enumerate() { | ||
| input.ordinal = first_ordinal + offset; |
There was a problem hiding this comment.
When a wildcard projection precedes a SQLPage-computed column, this ordinal is based on AST select-item count, but the database expands */table.* into all real columns before returning rows. For SELECT *, sqlpage.url_encode(name) AS enc FROM users with more than one users column, row_to_json_with_inputs will treat the second visible column as the private input, omit it from the row, and leave the generated __sqlpage_input_* column visible (or feed the wrong value to the function).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c84eb68d77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| build_sqlpage_expr::<Environment>(rewriter, *right)?, | ||
| ] | ||
| .into_boxed_slice(), | ||
| null_behavior: ConcatNullBehavior::PropagateNull, |
There was a problem hiding this comment.
On Oracle and SQL Server pages that use || in a SQLPage-owned or single-row fast-path expression, such as SELECT '/' || NULL AS path or sqlpage.url_encode('/' || nullable_col), this hard-codes null propagation and returns NULL. Those backends previously evaluated the operator in the database (or as CONCAT for SQL Server), where NULL is ignored, so the result should be /; the concat() helper already uses database.concat_function_null_behavior(), and || needs the same backend-specific handling or should remain a database input.
Useful? React with 👍 / 👎.
| .iter() | ||
| .map(|column| column.name.as_str()) | ||
| .collect::<Vec<_>>(); | ||
| if let Some(order_by) = &query.order_by |
There was a problem hiding this comment.
For newly supported computed projections wrapped in an emulated function, e.g. SELECT coalesce(sqlpage.url_encode(name), '') AS enc, count(*) FROM users GROUP BY enc, this rewrite removes enc from the database select list but only checks ORDER BY. On backends that allow GROUP BY to reference a select alias, the rewritten SQL groups by a name that no longer exists (or by an underlying enc column if present), so it fails or groups the wrong data instead of producing the grouped result; add the same alias/ordinal rejection for GROUP BY before sending the SQL to the database.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a35fa1e0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(kind) = emulated_function(&function) { | ||
| return self.rewrite_emulated_projection(function, kind); |
There was a problem hiding this comment.
When a top-level projection is an ordinary database JSON_OBJECT using PostgreSQL/SQL-standard named-argument syntax such as JSON_OBJECT('name': name) or JSON_OBJECT('name' VALUE name), this branch classifies it as emulated before checking whether any SQLPage-owned child exists. take_expression_arguments then rejects ExprNamed, so documented database JSON queries now fail during SQLPage rewrite instead of being passed through to the database; only enter the emulation path after confirming a SQLPage-computed child, or rebuild named arguments unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a175fd4215
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4052d3b83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lovasoa
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2276b31b91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let SetExpr::Select(select) = query.body.as_mut() else { | ||
| return Ok(Vec::new()); | ||
| }; | ||
| if select.distinct.is_some() && select.projection.iter().any(select_item_contains_sqlpage) { |
There was a problem hiding this comment.
For newly supported computed projections under an emulated parent, SELECT coalesce(sqlpage.url_encode(name), '') AS enc INTO out FROM t passes this guard. The rewrite later replaces enc with hidden inputs/anchors, and SELECT INTO writes that rewritten projection to the target table instead of returning rows for SQLPage to compute, so the created table gets internal columns rather than the requested enc value. Reject SQLPage-computed projections when select.into is set.
Useful? React with 👍 / 👎.
| if let Some(order_by) = order_by | ||
| && let OrderByKind::Expressions(expressions) = &order_by.kind | ||
| { | ||
| reject_computed_ordering_references("ORDER BY", expressions, computed_columns)?; |
There was a problem hiding this comment.
On dialects that support ORDER BY ALL such as DuckDB/Snowflake, this guard only inspects explicit order expressions. For a newly supported computed projection like SELECT coalesce(sqlpage.url_encode(name), '') AS enc FROM t ORDER BY ALL, the SQL sent to the database projects the hidden raw name input instead of enc, so the database orders by hidden inputs rather than the final SQLPage-computed values. Treat OrderByKind::All as invalid whenever computed columns are present.
Useful? React with 👍 / 👎.
| let GroupByExpr::Expressions(expressions, _) = group_by else { | ||
| return Ok(()); |
There was a problem hiding this comment.
For dialects with GROUP BY ALL, this non-Expressions branch lets computed columns through after they have been replaced by private row inputs. A query such as SELECT coalesce(sqlpage.regex_match(name, '^a'), '0') AS bucket, count(*) FROM t GROUP BY ALL is then grouped by the raw hidden name input instead of the computed bucket, producing the wrong aggregate groups. Reject GROUP BY ALL when computed_columns is non-empty.
Useful? React with 👍 / 👎.
1 participant
SQLPage functions that are the whole value of a selected column now execute after the database query, once per returned row. If the query returns no rows, the function is not called. This ensures deterministic behavior and prevents expensive or side-effectful operations from running unnecessarily.