Skip to content

Centralize the __giql_ reserved-namespace contract and reserved-identifier constant convention #178

Description

@conradbzura

Context

Surfaced during the round-1 review of PR #177 (Closes #161) as advisory findings A1 (the "Other" remediation — repo-wide cleanup) and A6 (the reserved-namespace contract). #161 moved the CLUSTER/MERGE synthesized identifiers into the reserved __giql_ namespace and extracted the two producer→consumer pairs (__giql_is_new_cluster, __giql_cluster_id) to CLUSTER_FLAG_COL / CLUSTER_ID_COL constants. This follow-up captures the two epic-level cleanups that were deliberately kept out of #161's scope.

A6 — The __giql_ reserved-namespace contract is undocumented and unguarded for CLUSTER/MERGE

__giql_ is the reserved prefix for every synthesized identifier across epic #137 (DISJOIN's __giql_dj_*, the canonicalizer's __giql_canon_*, the expander's __giql_x_*, and CLUSTER/MERGE's __giql_lag_calc / __giql_is_new_cluster / __giql_cluster_id / __giql_clustered), but the guarantee is neither documented for users nor uniformly enforced:

  • Only DISJOIN's __giql_dj_ prefix is guarded — the resolver rejects a user reference that starts with DJ_PREFIX (src/giql/resolver.py, the reserved-prefix guard). CLUSTER/MERGE synthesize their reserved names with no guard: a user column literally named __giql_is_new_cluster still silently re-collides (confirmed: cumulative SUM mis-binds and returns [7,14,21] instead of [1,1,2]).
  • There is no user-facing statement anywhere that __giql_-prefixed identifiers are reserved.

This is a genuine epic-level gap, not a regression #161 introduced — the pre-existing __giql_cluster_id already depended on the same unguarded namespace, and trading a plausible user column (is_new_cluster) for an implausible one (__giql_is_new_cluster) was the right scope call for #161.

Follow-up:

  1. Document the __giql_ reserved-namespace contract in one place — a single statement of the guarantee plus a table of which subsystems own which sub-namespaces (__giql_dj_ → DISJOIN, __giql_canon_ → canonicalizer, __giql_x_ → expander aliases, __giql_lag_calc / __giql_is_new_cluster / __giql_cluster_id / __giql_clustered → CLUSTER/MERGE). Add a one-line note in docs/transpilation/index.rst that __giql_-prefixed identifiers are reserved and must not appear in user tables/queries.
  2. Consider extending the resolver's reserved-prefix guard so it covers CLUSTER/MERGE FROM sources the way it already covers DISJOIN, so a user column/relation in the reserved namespace is rejected loudly rather than silently mis-binding.

A1 (Other) — Repo-wide reserved-identifier constant convention

Epic #137 mixes two conventions for reserved identifiers: named module constants (DJ_PREFIX in constants.py, CANON_PREFIX in canonicalizer.py, EXPAND_ALIAS_PREFIX in expander.py) versus inline string literals. #161 extracted the two CLUSTER/MERGE producer→consumer pairs to constants (CLUSTER_FLAG_COL, CLUSTER_ID_COL) and left the single-use aliases (__giql_lag_calc, __giql_clustered) inline, matching the reviewers' recommended (smaller) remediation.

Follow-up: Sweep the repo for remaining inline reserved-identifier literals and decide a single convention — either a constants.py-owned name per reserved identifier (or a CL_PREFIX = "__giql_cl_" subsystem prefix for CLUSTER/MERGE, symmetric with DJ_PREFIX, which would also give the single-use aliases a source of truth) — and apply it uniformly. This overlaps with A6's centralization: the sub-namespace table and the constants can share one home.

References

https://claude.ai/code/session_01KAYsMtN7vYuECZHeeFFzCM

Metadata

Metadata

Assignees

No one assigned

    Labels

    refactorCode restructuring without behavior change

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions