Skip to content

fix: write a comprehension's iteration variable through the dialect - #169

Open
scottlaird wants to merge 1 commit into
SPANDigital:mainfrom
scottlaird:fix/json-each-iteration-variable
Open

fix: write a comprehension's iteration variable through the dialect#169
scottlaird wants to merge 1 commit into
SPANDigital:mainfrom
scottlaird:fix/json-each-iteration-variable

Conversation

@scottlaird

Copy link
Copy Markdown

Fixes #168.

json_each and JSON_TABLE are table-valued functions, so the alias a comprehension binds names a row rather than the array's element:

-- approvals.exists(a, a == "alice")
EXISTS (SELECT 1 FROM json_each(approvals) AS a WHERE a = ?)

SQLite rejects that with no such column: a, and MySQL the same way — after the converter has reported the conversion a success. That is the awkward part: nothing in the caller's path says the SQL is wrong, and the failure arrives at query time.

The fix

A reference to an iteration variable is now written by the dialect, which is the only thing that knows what its own comprehension source bound the alias to.

// WriteIterVarRef writes a reference to a comprehension's iteration
// variable, given the alias the source was bound to.
WriteIterVarRef(w *strings.Builder, alias string) error
  • BigQuery, DuckDB, PostgreSQL, Spark write the bare alias: UNNEST and EXPLODE bind the element itself.
  • SQLite and MySQL qualify it, unconditionally — both of their comprehension sources are the table-valued form. WriteUnnest and WriteJSONArrayElements in dialect/sqlite both emit json_each, so there is no case where the alias is a value there. (That is also why the first version of this patch, which passed a jsonSource flag down from the converter, was wrong: it left the plain-array path generating a for a json_each source.)

The converter tracks which names are iteration variables so only those are routed that way, scoped to the comprehension and restored afterwards, since comprehensions nest and an inner one may reuse a name.

One detail worth pointing at: the filter comprehension writes the variable into its projection, ahead of the FROM clause, so the binding is taken at the top of each visitor rather than after the source is written — and that projection goes through WriteIterVarRef too.

Scope

all, exists, exists_one, filter and map all go through the same source writer, so all five were affected.

Tests

sqlite_iteration_variable_test.go converts and then executes against an in-memory SQLite database, asserting the rows that come back. That is the check this bug asks for: the previous expectations were SQL that parsed and could not run. Against main it fails with no such column: a.

The shared SQLite expectations in testcases/comprehension_tests.go encoded the old output and are updated; they are the same five comprehensions.

go test ./... is otherwise unchanged from main on this machine — the 27 failures before and after are the testcontainers suites, which need Docker.

Note on the MySQL change

I included MySQL because WriteUnnest there emits JSON_TABLE(… COLUMNS(value TEXT PATH '$')), which has the same shape. I could not execute against MySQL here (no Docker), so that half is reasoned rather than run — happy to split it out if you would rather take SQLite alone first.

json_each and JSON_TABLE are table-valued functions, so the alias a
comprehension binds names a row rather than the array's element:

  approvals.exists(a, a == "alice")
    -> EXISTS (SELECT 1 FROM json_each(approvals) AS a WHERE a = ?)

SQLite rejects that with "no such column: a", and MySQL the same way, after
the converter has reported the conversion a success. The failure appears at
query time rather than at conversion time, which is the awkward part: nothing
in the caller's path says the SQL is wrong.

A reference to an iteration variable is now written by the dialect, which is
the only thing that knows what its own comprehension source bound the alias
to. Dialect.WriteIterVarRef defaults to the bare alias -- correct for
BigQuery, DuckDB, Postgres and Spark, where UNNEST and EXPLODE bind the
element itself -- and SQLite and MySQL qualify it, unconditionally, because
both of their sources are the table-valued form.

The converter tracks which names are iteration variables so that only those
are routed that way, scoped to the comprehension and restored afterwards,
since comprehensions nest and an inner one may reuse a name. The filter
comprehension writes the variable into its projection ahead of the FROM
clause, so the binding is taken at the top of each visitor rather than after
the source is written.

The shared SQLite expectations in testcases/comprehension_tests.go encoded the
old output and are updated. sqlite_iteration_variable_test.go is new and
executes what it converts against an in-memory database, which is the check
this bug asks for: the previous expectations were SQL that parsed and could
not run.

Fixes SPANDigital#168
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.

cel2sql generates invalid SQLite for JSON columns

1 participant