Use the reserved __giql_ prefix for CLUSTER/MERGE synthesized identifiers — Closes #161#177
Merged
Merged
Conversation
The CLUSTER and MERGE expanders synthesized three internal SQL identifiers outside the reserved __giql_ namespace: the per-row is_new_cluster flag, the CLUSTER derived-table alias lag_calc, and the MERGE derived-table alias clustered. A user table projecting a real column named is_new_cluster collided with the synthesized flag — the inner subquery exposed two is_new_cluster columns and the outer SUM(is_new_cluster) OVER (...) bound to the wrong one, executing silently and returning corrupted cluster ids (or a binder error for a non-integer column). Move all three into the reserved __giql_ namespace so they cannot collide with user-supplied names, matching the already-prefixed __giql_cluster_id sibling. Update the emitted-SQL example in the transpilation docs, which had drifted to a half-migrated state. Claude-Session: https://claude.ai/code/session_01KAYsMtN7vYuECZHeeFFzCM
Update the transpilation-string assertions that pinned the old bare lag_calc / clustered / is_new_cluster names to their reserved __giql_ forms. Add two executable DuckDB regressions — a star-projected CLUSTER and a MERGE, each over a table carrying a user is_new_cluster column — asserting correct cluster ids and merged rows, which proves the rename disambiguates end-to-end (the collision executed silently and returned wrong results before). Add a negative-leak guard per operator pinning that no bare synthesized alias is emitted. Claude-Session: https://claude.ai/code/session_01KAYsMtN7vYuECZHeeFFzCM
d05ed30 to
53f9a1a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move the three CLUSTER/MERGE synthesized SQL identifiers into the reserved
__giql_namespace so they cannot collide with user-supplied names. The per-row flagis_new_cluster, the CLUSTER derived-table aliaslag_calc, and the MERGE derived-table aliasclusteredpreviously lived outside the namespace epic #137 reserves for synthesized names (giql/expander.py,EXPAND_ALIAS_PREFIX), unlike the already-prefixed__giql_cluster_id.The genuine bug is the
is_new_clusterflag — a projected column that coexists withSELECT *. A user table projecting a real column namedis_new_clustercollided with the synthesized flag: the inner subquery exposed twois_new_clustercolumns and the outerSUM(is_new_cluster) OVER (...)bound to the wrong one, executing silently and returning corrupted cluster ids for an integer column (or aBinderExceptionfor a non-integer one). Thelag_calc/clusteredrenames are namespace hygiene: those are derived-table aliases in a fresh SQL scope, so they never actually shadowed a same-named user column or relation at runtime, but the reserved prefix makes the intent explicit and guards against future reuse.This intentionally changes the emitted SQL for CLUSTER and MERGE (the synthesized alias names only), so the transpilation-string assertions and the transpilation docs' emitted-SQL example are updated in lockstep.
Closes #161
Proposed changes
Reserved-prefix rename in the CLUSTER and MERGE expanders
Rename the synthesized identifiers to
__giql_is_new_cluster/__giql_lag_calc(src/giql/expanders/cluster.py) and__giql_clustered(src/giql/expanders/merge.py), along with their docstring and comment references. Each name is emitted from a single unconditional code path in its expander, so the prefix holds across every parameter variant (stranded, max-gap, predicate, nested).Shared constants for the two producer→consumer names
Extract the two names that are aliased in one place and consumed in another —
__giql_is_new_cluster(aliased in the inner CTE, consumed by the outerSUMwindow) and__giql_cluster_id(aliased by MERGE's CLUSTER, consumed by theGROUP BY) — toCLUSTER_FLAG_COLandCLUSTER_ID_COLinsrc/giql/constants.py, and consume them fromcluster.py/merge.py. This mirrors howdisjoin.pyimportsDJ_PREFIXfrom the same module, giving each producer→consumer pair a single source of truth so a one-sided typo cannot silently reintroduce the binder error. The single-use derived-table aliases (__giql_lag_calc,__giql_clustered) stay inline.Transpilation-docs oracle fix
docs/transpilation/index.rstshowed a half-migrated MERGE emitted-SQL example —__giql_cluster_idalready prefixed alongside bareis_new_cluster/lag_calc/clustered. Update those lines to match the actual transpiler output, and quote theGROUP BY "chrom"term the transpiler actually emits.Test cases
Update the existing transpilation-string assertions across
tests/expanders/test_cluster.py,tests/expanders/test_merge.py, andtests/test_expander.pyfrom the old bare names to the__giql_forms, and add the coverage below.TestClusterExpanderis_new_clusterand a star-projected CLUSTERTestMergeExpanderis_new_clusterand a MERGETestClusterExpander__giql_lag_calc/__giql_is_new_clusterand emits no bare formsTestMergeExpander__giql_clustered/__giql_lag_calc/__giql_is_new_clusterand emits no bare formsReview
Round-1 review (
.sdlc/reviews/issue-#161/review-1.md) returned zero blocking findings across nine reviewers; all findings were advisory. Remediated here: extracting the two producer→consumer names to shared constants (A1), softening thelag_calc/clusteredcomments to describe reserved-namespace hygiene rather than a runtime shadowing bug that cannot occur (A2), fixing twin comment/docstring drift (A3), quotingGROUP BY "chrom"in the docs oracle (A4), reflowing orphaned comment/docstring fragments (A5), and tightening the AAA phase boundary in the two executable tests (A8). The undocumented__giql_namespace contract and a repo-wide reserved-identifier constant convention are deferred to follow-up #178.https://claude.ai/code/session_01KAYsMtN7vYuECZHeeFFzCM