Skip to content

Commit 1250196

Browse files
renecannaorenecannao
committed
docs(issues): publish local backlog and add issues 08, 09
The docs/issues/ directory was tracked locally but never committed. This publishes the seven prior-session issues (01-07, status notes unchanged) plus two new ones found by scripts/test_sqlengine.sh: - 08 (P1): aggregate projection schema is wrong in single-shard mode. SELECT COUNT(*)/SUM(...) against a 1-shard sharded-table config shows catalog table columns as result headers and puts the aggregate value in the wrong slot. 2-shard mode is unaffected. - 09 (P0): single-shard route by shard key returns wrong (empty) results for some integer keys. WHERE id=5 against the demo two-shard set returns empty even though Eve lives on shard1; WHERE id=7 returns Grace correctly. Either DistributedPlanner ignores the shard key or the router and the demo data placement disagree. Wrong-results-without-error path. Cross-referenced from docs/architecture-and-status.md §8.
1 parent 2ee4128 commit 1250196

11 files changed

Lines changed: 566 additions & 0 deletions

‎docs/architecture-and-status.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,8 @@ Source: `docs/issues/README.md`. Status reflects the working tree on 2026-04-17.
572572

573573
1. **[01] Distributed 2PC must require safe session pinning** — 🟦 in working tree.
574574
*Add `allows_unpinned_distributed_2pc()` flag. Default false. Pooled executors without pinning cannot enlist.*
575+
9. **[09] Single-shard route by shard key misdirects for some keys** — 📋 open. Surfaced 2026-04-18 by `scripts/test_sqlengine.sh sharded`.
576+
*`WHERE id = 5` against `users:id:shard1,shard2` returns empty rows; `id = 7` works. Wrong-results-without-error path. Either the router and the demo's data placement disagree, or `DistributedPlanner` ignores the shard key.*
575577

576578
### P1 — important correctness & maintainability
577579

@@ -581,6 +583,8 @@ Source: `docs/issues/README.md`. Status reflects the working tree on 2026-04-17.
581583
*New module `tool_config_parser.{h,cpp}`. Replaces ~135 lines of duplication in each of `sqlengine`, `mysql_server`, `bench_distributed`, `engine_stress_test`, plus `tests/test_ssl_config.cpp`.*
582584
4. **[04] Close join execution coverage gaps** — 🟦 in working tree.
583585
*`NestedLoopJoinOperator` now executes RIGHT and FULL outer joins (with right-row match tracking) and resolves qualified column names in join conditions.*
586+
8. **[08] Aggregate projection schema wrong in single-shard mode** — 📋 open. Surfaced 2026-04-18 by `scripts/test_sqlengine.sh single`.
587+
*`SELECT COUNT(*)` / `SELECT SUM(salary)` against a 1-shard config show catalog table columns as headers and put the aggregate value in the wrong slot. Two-shard mode is unaffected.*
584588

585589
### P2 — deferred
586590

‎docs/issues/01-distributed-2pc-safe-session-pinning.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# Issue 01: Distributed 2PC Must Require Safe Session Pinning
2+
3+
## Priority
4+
5+
`P0`
6+
7+
## Status
8+
9+
Implemented in the current working tree; not yet committed.
10+
11+
## Problem
12+
13+
`DistributedTransactionManager` currently falls back to unpinned `execute_dml()` calls when `checkout_session()` returns `nullptr`. That fallback is explicitly documented as unsafe for pooled real-backend 2PC because phase 1 and phase 2 may run on different physical connections.
14+
15+
## Evidence
16+
17+
- [include/sql_engine/remote_executor.h](/data/rene/ParserSQL/include/sql_engine/remote_executor.h:25)
18+
- [include/sql_engine/distributed_txn.h](/data/rene/ParserSQL/include/sql_engine/distributed_txn.h:110)
19+
- [include/sql_engine/distributed_txn.h](/data/rene/ParserSQL/include/sql_engine/distributed_txn.h:141)
20+
21+
## Risk
22+
23+
- Silent correctness failure in distributed transactions
24+
- Broken XA / `PREPARE TRANSACTION` behavior when executors are pooled
25+
- Hard-to-debug commit/rollback divergence across participants
26+
27+
## Desired Outcome
28+
29+
Distributed 2PC must fail closed unless the executor can prove one of these is true:
30+
31+
1. It provides pinned sessions via `checkout_session()`
32+
2. It explicitly declares that unpinned fallback is safe for its execution model
33+
34+
## Scope
35+
36+
- Add an executor capability check to the `RemoteExecutor` API
37+
- Update `DistributedTransactionManager` to reject unsafe fallback paths
38+
- Keep single-connection executors and test mocks usable by explicitly opting them into legacy-safe fallback
39+
- Update tests that currently assume all `nullptr` session paths are acceptable
40+
41+
## Acceptance Criteria
42+
43+
- A pooled executor without pinned session support cannot enlist or route DML for distributed 2PC
44+
- Executors that guarantee single-connection-per-backend behavior can still opt into the fallback path
45+
- Existing pinned-session behavior remains unchanged
46+
- Regression tests cover both safe and unsafe fallback cases
47+
48+
## Verification
49+
50+
- Targeted unit tests in `tests/test_distributed_txn.cpp`
51+
- Targeted unit tests in `tests/test_session.cpp` if route behavior changes

‎docs/issues/02-distributed-2pc-deterministic-phase-timeouts.md‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# Issue 02: Make 2PC Phase Timeouts Deterministic
2+
3+
## Priority
4+
5+
`P1`
6+
7+
## Status
8+
9+
Implemented in the current working tree.
10+
11+
## Problem
12+
13+
Per-phase timeout handling in `DistributedTransactionManager` is currently documented as best-effort. The timeout setter may run as a separate statement from the XA/PREPARE/COMMIT statement it is intended to bound, which means the timeout can miss the actual protected operation.
14+
15+
## Evidence
16+
17+
- [include/sql_engine/distributed_txn.h](/data/rene/ParserSQL/include/sql_engine/distributed_txn.h:59)
18+
19+
## Risk
20+
21+
- Coordinator hangs or long stalls during prepare/commit on unhealthy backends
22+
- False sense of safety from a timeout API that is not deterministic
23+
24+
## Desired Outcome
25+
26+
Timeout semantics should be deterministic for the statement they claim to bound. If deterministic behavior cannot be guaranteed for a backend/executor combination, the API should either reject the configuration or document and surface the weaker behavior explicitly.
27+
28+
## Scope
29+
30+
- Decide the timeout model by backend and executor type
31+
- Remove or sharply constrain ambiguous “best-effort” behavior
32+
- Add tests around timeout application sequencing if feasible
33+
34+
## Acceptance Criteria
35+
36+
- The effective timeout behavior is explicit and reliable
37+
- API behavior is consistent across supported 2PC executors
38+
- Documentation and tests match the final semantics
39+
40+
## Resolution Notes
41+
42+
- PostgreSQL phase timeouts now use `SET statement_timeout = <ms>` immediately before `PREPARE TRANSACTION` and `COMMIT/ROLLBACK PREPARED` on the participant session.
43+
- MySQL no longer emits `SET SESSION max_execution_time` for XA phase SQL, because that setting does not bound XA control statements.
44+
- Enlistment no longer injects timeout SQL before `BEGIN` / `XA START`.
45+
46+
## Verification
47+
48+
- `make clean && make run_tests && ./run_tests --gtest_filter="DistributedTxnTimeout.*" --gtest_brief=1`
49+
- `./run_tests --gtest_brief=1`

‎docs/issues/03-shared-backend-config-parsing.md‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
# Issue 03: Extract Shared Backend And Shard Configuration Parsing
2+
3+
## Priority
4+
5+
`P1`
6+
7+
## Status
8+
9+
Implemented in the current working tree.
10+
11+
## Problem
12+
13+
Backend URL parsing and shard spec parsing are duplicated across multiple tools. The code paths are intended to stay aligned, but they are maintained by copy-paste.
14+
15+
## Evidence
16+
17+
- [tools/sqlengine.cpp](/data/rene/ParserSQL/tools/sqlengine.cpp:203)
18+
- [tools/mysql_server.cpp](/data/rene/ParserSQL/tools/mysql_server.cpp:65)
19+
- [tools/bench_distributed.cpp](/data/rene/ParserSQL/tools/bench_distributed.cpp:51)
20+
- [tools/engine_stress_test.cpp](/data/rene/ParserSQL/tools/engine_stress_test.cpp:53)
21+
22+
## Risk
23+
24+
- Tool behavior diverges over time
25+
- SSL, naming, or shard parsing bugs are fixed in one entry point but not others
26+
- Extra friction for integration testing and documentation
27+
28+
## Desired Outcome
29+
30+
One shared parser for backend URLs and shard specs, reused by all tool entry points and test helpers.
31+
32+
## Scope
33+
34+
- Extract reusable parsing helpers
35+
- Update all current tool entry points to use the shared code
36+
- Add focused unit tests for parsing behavior
37+
38+
## Acceptance Criteria
39+
40+
- No copy-pasted backend/shard parsing remains across tool binaries
41+
- Existing CLI behavior is preserved
42+
- Parsing behavior is covered by direct tests
43+
44+
## Resolution Notes
45+
46+
- Added a shared parser interface in `include/sql_engine/tool_config_parser.h`.
47+
- Implemented shared backend URL and shard spec parsing in `src/sql_engine/tool_config_parser.cpp`.
48+
- Updated `sqlengine`, `mysql_server`, `bench_distributed`, and `engine_stress_test` to use the shared parser instead of local copies.
49+
- Switched `tests/test_ssl_config.cpp` to hit the shared parser directly and added shard parsing coverage.
50+
51+
## Verification
52+
53+
- `make run_tests && ./run_tests --gtest_filter="SSLConfigTest.*" --gtest_brief=1`
54+
- `make build-sqlengine bench-distributed engine-stress mysql-server`
55+
- `./run_tests --gtest_brief=1`

‎docs/issues/04-join-operator-coverage.md‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# Issue 04: Close Join Execution Coverage Gaps Or Reject Unsupported Joins Earlier
2+
3+
## Priority
4+
5+
`P1`
6+
7+
## Status
8+
9+
Implemented in the current working tree.
10+
11+
## Problem
12+
13+
The parser and planner can represent a broader join surface than the executor can run. Execution currently rejects `RIGHT` and `FULL` joins at runtime, and hash join only supports `INNER` and `LEFT`.
14+
15+
## Evidence
16+
17+
- [include/sql_engine/operators/join_op.h](/data/rene/ParserSQL/include/sql_engine/operators/join_op.h:33)
18+
- [include/sql_engine/operators/hash_join_op.h](/data/rene/ParserSQL/include/sql_engine/operators/hash_join_op.h:37)
19+
20+
## Risk
21+
22+
- Runtime failures after successful parse/plan
23+
- Confusing user-facing capability boundary
24+
- Incomplete planner/executor contract
25+
26+
## Desired Outcome
27+
28+
Either implement the missing join forms end-to-end, or reject them consistently earlier with explicit error surfaces.
29+
30+
## Scope
31+
32+
- Decide whether to implement or early-reject `RIGHT` and `FULL`
33+
- Align planner, executor, and tests around the same contract
34+
- Preserve existing `INNER`, `LEFT`, and `CROSS` semantics
35+
36+
## Acceptance Criteria
37+
38+
- Unsupported joins do not make it deep into execution silently
39+
- Supported joins are documented and tested consistently
40+
41+
## Resolution Notes
42+
43+
- Extended `NestedLoopJoinOperator` to implement `RIGHT` and `FULL` join semantics instead of rejecting them at runtime.
44+
- Added right-side match tracking so unmatched right rows are emitted correctly for `RIGHT` and `FULL`.
45+
- Fixed nested-loop join predicate resolution for qualified names such as `users.id` and `orders.user_id`.
46+
- Added operator-level and plan-executor coverage for `RIGHT` and `FULL` joins.
47+
48+
## Verification
49+
50+
- `rm -f tests/test_operators.o tests/test_plan_executor.o run_tests && make run_tests && ./run_tests --gtest_filter="JoinOpTest.RightJoinIncludesUnmatchedRightRows:JoinOpTest.FullJoinIncludesUnmatchedRowsFromBothSides:HashJoinTest.RightJoinIncludesUnmatchedRightRows:HashJoinTest.FullJoinIncludesUnmatchedRowsFromBothSides" --gtest_brief=1`
51+
- `make clean && make run_tests && ./run_tests --gtest_brief=1`

‎docs/issues/05-expression-and-type-semantics.md‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
# Issue 05: Tighten Expression And Type Semantics
2+
3+
## Priority
4+
5+
`P2`
6+
7+
## Status
8+
9+
In progress.
10+
11+
## Problem
12+
13+
Several expression and type paths still return `NULL` or simplified behavior where real semantics are expected. This includes tuple values, non-literal arrays, composite field access, simplified string length behavior, and string-backed decimals.
14+
15+
## Evidence
16+
17+
- [include/sql_engine/expression_eval.h](/data/rene/ParserSQL/include/sql_engine/expression_eval.h:688)
18+
- [include/sql_engine/functions/string.h](/data/rene/ParserSQL/include/sql_engine/functions/string.h:70)
19+
- [include/sql_engine/value.h](/data/rene/ParserSQL/include/sql_engine/value.h:21)
20+
- [include/sql_engine/functions/cast.h](/data/rene/ParserSQL/include/sql_engine/functions/cast.h:43)
21+
22+
## Risk
23+
24+
- Dialect inconsistencies
25+
- Incorrect or lossy semantics for non-trivial expressions
26+
- Limited headroom for richer type support
27+
28+
## Desired Outcome
29+
30+
Close the most user-visible semantic gaps first, with explicit tests per behavior.
31+
32+
## Scope
33+
34+
- Prioritize UTF-8-aware length semantics, clearer array behavior, and targeted cast improvements
35+
- Defer large type-system expansions unless directly needed by query behavior
36+
37+
## Acceptance Criteria
38+
39+
- Unsupported paths are reduced or explicitly surfaced
40+
- Added semantics are covered by targeted unit tests
41+
42+
## Progress Notes
43+
44+
- `CHAR_LENGTH` now counts UTF-8 code points, while `LENGTH` remains byte-based.
45+
- PostgreSQL explicit casts now accept `on` / `off` for boolean strings and support string-to-`DATE` / `TIME` / `DATETIME` / `TIMESTAMP` helper paths.
46+
- Array constructors, tuple standalone values, composite field access, and string-backed decimal semantics remain open.
47+
48+
## Verification So Far
49+
50+
- `rm -f tests/test_string_funcs.o tests/test_eval_integration.o src/sql_engine/function_registry.o run_tests && make run_tests && ./run_tests --gtest_filter="StringFuncTest.*Length*:EvalIntegrationTest.SelectLength*:EvalIntegrationTest.SelectCharLengthUsesCodePointCountForUtf8:EvalIntegrationTest.PgSelectCharLengthUsesCodePointCountForUtf8" --gtest_brief=1`
51+
- `rm -f tests/test_cast.o run_tests && make run_tests && ./run_tests --gtest_filter="CastTest.PgSQL*:CastTest.UnsupportedTarget" --gtest_brief=1`
52+
- `./run_tests --gtest_brief=1`

‎docs/issues/06-parser-gaps-select-into-and-recursive-cte.md‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# Issue 06: Close Parser Gaps Around `SELECT ... INTO` And Recursive CTE Handling
2+
3+
## Priority
4+
5+
`P2`
6+
7+
## Problem
8+
9+
A few parser paths are still marked as skipped or future work. Notable examples are MySQL `SELECT ... INTO` placement variants and recursive CTE handling that is currently only marked in flags without complete downstream semantics.
10+
11+
## Evidence
12+
13+
- [include/sql_parser/select_parser.h](/data/rene/ParserSQL/include/sql_parser/select_parser.h:43)
14+
- [src/sql_parser/parser.cpp](/data/rene/ParserSQL/src/sql_parser/parser.cpp:1223)
15+
- [tests/test_set.cpp](/data/rene/ParserSQL/tests/test_set.cpp:237)
16+
17+
## Risk
18+
19+
- Partial dialect coverage in edge syntax
20+
- Features appear parsed but are not fully executed or planned
21+
22+
## Desired Outcome
23+
24+
Document and close parser-specific surface gaps independently of executor-wide CTE integration work.
25+
26+
## Scope
27+
28+
- Add parser coverage for skipped `SELECT ... INTO` forms
29+
- Decide whether recursive CTEs should be parsed-only, rejected, or supported end-to-end
30+
- Remove stale comments once actual behavior is settled
31+
32+
## Acceptance Criteria
33+
34+
- Parser behavior matches the documented syntax contract
35+
- Tests exist for each newly supported or explicitly rejected form
36+

‎docs/issues/07-session-cte-integration.md‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# Issue 07: Integrate CTE Handling Into The Main `Session` Query Path
2+
3+
## Priority
4+
5+
`P2`
6+
7+
## Problem
8+
9+
CTE execution exists through `PlanExecutor::execute_with_cte()`, but the main `Session::execute_query()` path does not use it. `PlanBuilder::build_cte()` currently just builds the main query and ignores the CTE materialization path.
10+
11+
## Evidence
12+
13+
- [include/sql_engine/plan_executor.h](/data/rene/ParserSQL/include/sql_engine/plan_executor.h:105)
14+
- [include/sql_engine/session.h](/data/rene/ParserSQL/include/sql_engine/session.h:83)
15+
- [include/sql_engine/plan_builder.h](/data/rene/ParserSQL/include/sql_engine/plan_builder.h:396)
16+
- [tests/test_cte.cpp](/data/rene/ParserSQL/tests/test_cte.cpp:70)
17+
18+
## Risk
19+
20+
- `WITH` queries behave differently depending on entry path
21+
- CTE tests validate executor-special behavior rather than the main user API
22+
23+
## Desired Outcome
24+
25+
`Session::execute_query()` should execute CTE-wrapped queries through the same high-level API that users already rely on, with planner/executor responsibilities clearly defined.
26+
27+
## Scope
28+
29+
- Decide whether CTE materialization lives in `Session`, `PlanExecutor`, or a new orchestration layer
30+
- Make the main query path CTE-aware
31+
- Preserve existing non-CTE behavior and plan caching rules
32+
33+
## Acceptance Criteria
34+
35+
- `Session::execute_query()` can run current non-recursive CTE queries
36+
- Existing CTE tests can be migrated or expanded to exercise the main path
37+
- CTE support remains intentionally limited until recursive support is explicitly tackled
38+

0 commit comments

Comments
 (0)