feat: advanced join semantics, transactional integrity, and expanded SLT suite#14
Merged
feat: advanced join semantics, transactional integrity, and expanded SLT suite#14
Conversation
📝 WalkthroughWalkthroughAdds B+ Tree index creation/backfill and index-aware planning, RIGHT/FULL JOIN parsing and execution, connection-scoped QueryExecutor state, enhanced undo/rollback with index maintenance, LIMIT/OFFSET semantics change, and a comprehensive SLT test suite with runner improvements and new tests for joins, indexes, limits, and transactions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant QueryExec as QueryExecutor
participant Catalog
participant Heap as HeapTable
participant BTree as BTreeIndex
participant TM as TransactionManager
Client->>Server: CREATE INDEX ...;
Server->>QueryExec: execute(statement)
QueryExec->>Catalog: validate table/column metadata
Catalog-->>QueryExec: table metadata (columns, indexes)
QueryExec->>BTree: create index file/structure
BTree-->>QueryExec: created
QueryExec->>Heap: scan table tuples (backfill)
loop for each tuple
Heap-->>QueryExec: tuple (row, rid)
QueryExec->>BTree: insert(key, rid)
BTree-->>QueryExec: ack / error
end
QueryExec->>Catalog: register new index
QueryExec->>Server: return success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/transaction/transaction_manager.cpp (1)
166-169: Consider settingsuccess = falsewhen table metadata is not found.When
get_table_by_namereturnsstd::nullopt, the code silently continues without marking the rollback as failed. While this may be intentional for dropped tables, it could mask legitimate rollback failures. Consider logging a warning or tracking this condition.Proposed enhancement
auto table_meta_opt = catalog_.get_table_by_name(log.table_name); if (!table_meta_opt) { + std::cerr << "Rollback WARNING: Table metadata not found for " << log.table_name << "\n"; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/transaction/transaction_manager.cpp` around lines 166 - 169, When catalog_.get_table_by_name(log.table_name) returns std::nullopt in the rollback loop, do not silently continue: set the rollback success flag (success) to false and emit a clear warning (e.g., via the existing logger) noting the missing table and relevant log metadata; if dropped-table behavior should remain allowed, make this conditional or document it. Locate the check around table_meta_opt in transaction_manager.cpp (where get_table_by_name is called) and update it to set success = false and log a warning mentioning log.table_name and any identifying transaction/log ids before continuing.src/parser/parser.cpp (1)
250-269: Consider: Loop permits duplicate LIMIT/OFFSET clauses.The while-loop structure allows
LIMIT 5 LIMIT 10orOFFSET 2 OFFSET 3, silently overwriting with the last value. While not incorrect, most SQL parsers reject duplicates. This is a minor robustness consideration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parser/parser.cpp` around lines 250 - 269, The loop in parser.cpp that handles LIMIT/OFFSET (using consume(TokenType::Limit)/consume(TokenType::Offset) and calling stmt->set_limit / stmt->set_offset) should reject duplicate clauses instead of silently overwriting: add checks (either via stmt->has_limit()/has_offset() if available or local bool flags like limit_set/offset_set) before calling set_limit/set_offset, and if a duplicate is encountered return nullptr (or signal a parse error) so sequences like "LIMIT 5 LIMIT 10" or "OFFSET 2 OFFSET 3" are refused; update the while loop logic to perform this check and early-return on duplicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/executor/query_executor.cpp`:
- Around line 534-536: The call to table.get(rid, old_tuple) ignores its return
value, so failures can leave old_tuple uninitialized and break subsequent index
maintenance; update the code around table.get(rid, old_tuple) to capture and
check its return (e.g., bool or Status result) and handle failure immediately
(log/return/throw) before any use of old_tuple or the if (!old_tuple.empty())
check, ensuring index-update code only runs when table.get succeeded and
old_tuple is valid.
In `@src/transaction/transaction_manager.cpp`:
- Around line 185-193: The rollback code in transaction_manager.cpp is ignoring
failures from index operations by discarding index.remove() results (e.g., in
the loop creating storage::BTreeIndex index and calling
index.remove(tuple.get(pos), log.rid)); change this to check the return value
for success/failure, aggregate any failures (e.g., set a local boolean or status
variable), log contextual information (index name, column position, rid) on
failure, and propagate or return an error/status from the rollback routine so
dangling index entries are detected and handled; apply the same fix pattern to
the other index operation sites referenced (lines with index.remove/index.insert
calls) to ensure mandatory result checking for all index operations.
In `@tests/logic/indexes.slt`:
- Around line 24-30: The test's format specifier "IT" expects two columns but
the SQL block only SELECTs one column (id); fix by making the specifier match
the result shape—either change the format specifier from "IT" to "I" to indicate
a single integer column, or modify the query to return two columns (e.g., SELECT
id, name FROM idx_test ...) so that "IT" correctly maps to Integer, Text; update
whichever you choose consistently in the same test block.
In `@tests/logic/limit_offset.slt`:
- Around line 10-15: The test header "query IT" declares two columns (Integer,
Text) but the SQL uses "SELECT id" which returns a single column; update each
query header in tests/logic/limit_offset.slt from "query IT" to "query I" (or
alternatively change the SQL to return two columns, e.g., "SELECT id,
some_text") so the declared column types match the actual result columns for
every query in this file (look for the "query IT" lines and the corresponding
"SELECT id" statements against lim_off_test).
In `@tests/logic/slt_runner.py`:
- Around line 188-191: The print call that logs " Query failed with ERROR
status" uses an unnecessary f-string prefix; edit the print statement in
tests/logic/slt_runner.py (the block that prints FAILURE at file_path:start_line
and references variables file_path, start_line, sql, and failed_tests) to remove
the leading "f" so it becomes a normal string literal (i.e., change print(f"
Query failed with ERROR status") to print(" Query failed with ERROR status")).
---
Nitpick comments:
In `@src/parser/parser.cpp`:
- Around line 250-269: The loop in parser.cpp that handles LIMIT/OFFSET (using
consume(TokenType::Limit)/consume(TokenType::Offset) and calling stmt->set_limit
/ stmt->set_offset) should reject duplicate clauses instead of silently
overwriting: add checks (either via stmt->has_limit()/has_offset() if available
or local bool flags like limit_set/offset_set) before calling
set_limit/set_offset, and if a duplicate is encountered return nullptr (or
signal a parse error) so sequences like "LIMIT 5 LIMIT 10" or "OFFSET 2 OFFSET
3" are refused; update the while loop logic to perform this check and
early-return on duplicates.
In `@src/transaction/transaction_manager.cpp`:
- Around line 166-169: When catalog_.get_table_by_name(log.table_name) returns
std::nullopt in the rollback loop, do not silently continue: set the rollback
success flag (success) to false and emit a clear warning (e.g., via the existing
logger) noting the missing table and relevant log metadata; if dropped-table
behavior should remain allowed, make this conditional or document it. Locate the
check around table_meta_opt in transaction_manager.cpp (where get_table_by_name
is called) and update it to set success = false and log a warning mentioning
log.table_name and any identifying transaction/log ids before continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 445c99ae-6a63-4ed9-9c71-3242e00cab1d
📒 Files selected for processing (22)
README.mddocs/phases/README.mdinclude/executor/operator.hppinclude/executor/query_executor.hppinclude/parser/statement.hppinclude/parser/token.hppinclude/storage/heap_table.hppinclude/transaction/transaction.hppinclude/transaction/transaction_manager.hppsrc/executor/operator.cppsrc/executor/query_executor.cppsrc/network/server.cppsrc/parser/lexer.cppsrc/parser/parser.cppsrc/storage/heap_table.cppsrc/transaction/transaction_manager.cpptest_data/idx_id.idxtests/logic/indexes.slttests/logic/joins.slttests/logic/limit_offset.slttests/logic/slt_runner.pytests/logic/transactions.slt
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
tests/logic/slt_runner.py (2)
110-120: Validate the SLT file before opening the socket.A missing local
.sltfile still creates a live server session here and then returns without using it. Doing theos.path.exists()check first avoids the extra connection entirely.♻️ Proposed cleanup
def run_slt(file_path, port): client = CloudSQLClient(port=port) - try: - client.connect() - except Exception as e: - print(f"ERROR: Connection failed: {e}") - return False - if not os.path.exists(file_path): print(f"ERROR: File not found: {file_path}") return False + + try: + client.connect() + except Exception as e: + print(f"ERROR: Connection failed: {e}") + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/logic/slt_runner.py` around lines 110 - 120, Move the SLT file existence check ahead of opening the DB socket: in run_slt, call os.path.exists(file_path) first and return False with the error print if missing, and only then instantiate CloudSQLClient(port=port) and call client.connect(); this prevents creating a live server session when the .slt file is absent and keeps CloudSQLClient and connect() usage unchanged otherwise.
160-168: Accept explicitnosortquery headers.The code currently rejects
query ... nosortwith an error, even thoughnosortshould be treated as an explicit request for unsorted results (same as omitting the sort parameter entirely). The third token is validated against a whitelist that includesrowsortandvaluesortbut notnosort.Suggested fix
parts = line.split() types = parts[1] sort_mode = parts[2] if len(parts) > 2 else None + if sort_mode == 'nosort': + sort_mode = None if sort_mode and sort_mode not in ['rowsort', 'valuesort']: print(f"ERROR at {file_path}:{start_line}: Unsupported sort mode '{sort_mode}'") sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/logic/slt_runner.py` around lines 160 - 168, The query header handling in slt_runner.py rejects "nosort" even though it should be allowed; update the block that parses sort_mode (the branch where line.startswith('query') sets types and sort_mode) to accept 'nosort' as a valid token by either adding 'nosort' to the allowed whitelist or by normalizing sort_mode=='nosort' to None so it is treated the same as omitting the sort parameter; ensure the existing error message (using file_path and start_line) still triggers for any other unsupported token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/transaction/transaction.hpp`:
- Around line 10-12: The code currently uses debug-only asserts to validate
UndoLog::Type in the overloads that record undo-log entries, which allows
malformed undo entries in release builds; replace those assert(...) checks with
runtime validation: explicitly check the provided UndoLog::Type in the overloads
that accept it, and if invalid either throw a descriptive exception or return an
error status (do not rely on asserts), and consider adding dedicated API
wrappers (e.g., type-specific record_* methods) so callers cannot pass the wrong
type; update the undo-log write paths (the overloads that record undo entries)
to refuse/handle invalid types and include clear error messages to prevent
corrupt rollback/index cleanup.
In `@src/executor/query_executor.cpp`:
- Around line 630-668: The UPDATE path performs heap remove/insert and then
index writes (using table.remove, table.insert and apply_index_write) which can
leave heap and indexes inconsistent if an index write fails; make the sequence
transactional by either (A) performing index updates via the same
transactional/index API before making the heap change, or (B) wrapping
heap+index operations in a try/catch and rolling back heap changes on failure:
if apply_index_write fails after table.remove, re-insert op.old_tuple at op.rid
(or delete the newly inserted new_tid) and undo any index changes already
applied, then propagate the error; ensure log_manager_ / txn LSN updates
(LogRecord MARK_DELETE and txn->set_prev_lsn) are only committed after all index
writes succeed, and prefer using txn-aware index operations so apply_index_write
is atomic with the transaction.
- Around line 874-878: The current code skips adding a FilterOperator whenever
index_used is true, which drops non-indexed predicates; instead, after creating
the IndexScan (when index_used == true) compute the residual predicate =
original stmt.where() minus the equality predicate consumed by the IndexScan
(the predicate the IndexScan used for lookup), and if that residual is non-empty
wrap current_root with std::make_unique<FilterOperator>(std::move(current_root),
residual->clone()). Use the same symbols shown (stmt.where(), index_used,
current_root, FilterOperator, IndexScan) — ensure IndexScan exposes or returns
the consumed predicate so you can subtract it from stmt.where() and only apply
the remaining predicates as the residual filter.
- Around line 542-557: The heap is modified by table.remove(...) before the undo
log is created, so an exception from apply_index_write(...) (e.g., during the
loop over table_meta->indexes or IndexOp::Remove) leaves the heap changed with
no undo record; fix by creating and appending the undo log immediately after the
successful table.remove(rid, xmax) and before any index operations (refer to
table.remove, old_tuple, apply_index_write, IndexOp::Remove and the undo_log
creation site), and make the same change for INSERT and UPDATE code paths
(ensure undo_log is added right after the heap/tuple change and prior to any
index writes or anything that can throw, or alternatively wrap the heap
change+index updates+undo append in a transactional/RAII scope that rolls back
on exceptions).
In `@src/parser/parser.cpp`:
- Around line 254-270: The LIMIT/OFFSET parsing currently accepts any
TokenType::Number and then calls Token::as_int64(), allowing decimals like 1.5;
update the parsing branch that handles consume(TokenType::Limit) and
consume(TokenType::Offset) to verify the numeric token is an integer (not a
decimal) before calling next_token() -> as_int64(); if the token is a decimal
(or otherwise not an integer), return nullptr instead of coercing, ensuring
stmt->set_limit and stmt->set_offset only get integer values.
- Around line 129-149: The parser accepts bare JOIN and LEFT/RIGHT/FULL but not
the explicit INNER token, so add a branch that handles consume(TokenType::Inner)
similar to the existing branches: optionally consume TokenType::Outer (if you
want to mirror the other branches), then require consume(TokenType::Join) and
set join_type = SelectStatement::JoinType::Inner; update the chain around the
other consume(...) branches in parser.cpp (the block using consume and
SelectStatement::JoinType) to include this TokenType::Inner branch so "INNER
JOIN" parses the same as bare "JOIN".
In `@src/transaction/transaction_manager.cpp`:
- Around line 187-201: The rollback currently ignores table.get(...) failures
and proceeds with heap changes, risking dangling index entries; update each
branch that calls table.get(log.rid, tuple) (the blocks handling INSERT,
UPDATE-new-version, DELETE, UPDATE-old-version) so that if table.get(...)
returns false you set success = false and do not perform any subsequent heap
mutation (e.g., do not call physical_remove or restore the row) for that log
entry; specifically, after each failed table.get(log.rid, tuple) add success =
false and bail out of that branch (skip index maintenance and any
physical_remove/restore steps) so the rollback treats the missing tuple as an
error.
- Around line 160-162: undo_transaction() now returns a bool on rollback
success, but callers (notably abort()) ignore it; update abort() to check the
return value from TransactionManager::undo_transaction(Transaction*) and if it
returns false, stop the abort path: do not append an ABORT record, do not
release locks or push the txn into completed_transactions_, and instead
propagate failure (return false / error) so higher-level code knows rollback
failed; also find the other caller that invokes undo_transaction (the other site
reported) and apply the same check/propagation so rollback failures aren't
masked.
---
Nitpick comments:
In `@tests/logic/slt_runner.py`:
- Around line 110-120: Move the SLT file existence check ahead of opening the DB
socket: in run_slt, call os.path.exists(file_path) first and return False with
the error print if missing, and only then instantiate CloudSQLClient(port=port)
and call client.connect(); this prevents creating a live server session when the
.slt file is absent and keeps CloudSQLClient and connect() usage unchanged
otherwise.
- Around line 160-168: The query header handling in slt_runner.py rejects
"nosort" even though it should be allowed; update the block that parses
sort_mode (the branch where line.startswith('query') sets types and sort_mode)
to accept 'nosort' as a valid token by either adding 'nosort' to the allowed
whitelist or by normalizing sort_mode=='nosort' to None so it is treated the
same as omitting the sort parameter; ensure the existing error message (using
file_path and start_line) still triggers for any other unsupported token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4601ae8-b6c3-44d0-b38f-6d4b0edf55c9
📒 Files selected for processing (7)
include/transaction/transaction.hppsrc/executor/query_executor.cppsrc/parser/parser.cppsrc/transaction/transaction_manager.cpptests/logic/indexes.slttests/logic/limit_offset.slttests/logic/slt_runner.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/logic/limit_offset.slt
Comment on lines
+10
to
+12
| #include <cassert> | ||
| #include <mutex> | ||
| #include <optional> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether release builds define NDEBUG and inspect current add_undo_log call sites.
set -euo pipefail
echo "== Build definitions related to assert/NDEBUG =="
fd 'CMakeLists\.txt$|\.cmake$' . -X rg -n 'NDEBUG|CMAKE_(C|CXX)_FLAGS_RELEASE|target_compile_definitions|add_compile_definitions' || true
echo
echo "== Current add_undo_log call sites =="
rg -n -C2 '\badd_undo_log\s*\(' include src tests || trueRepository: poyrazK/cloudSQL
Length of output: 2228
🏁 Script executed:
sed -n '120,140p' include/transaction/transaction.hppRepository: poyrazK/cloudSQL
Length of output: 1034
🏁 Script executed:
cat CMakeLists.txt | head -50Repository: poyrazK/cloudSQL
Length of output: 1521
🏁 Script executed:
grep -i "ndebug\|release\|debug\|CMAKE_BUILD_TYPE" CMakeLists.txtRepository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
# Check if there are any compile-time constraints (templates, concepts, etc.) on add_undo_log
sed -n '60,140p' include/transaction/transaction.hpp | grep -A 5 -B 5 "add_undo_log\|concept\|enable_if"Repository: poyrazK/cloudSQL
Length of output: 1010
Don't use debug-only asserts to enforce undo-log invariants.
Lines 127 and 135 stop protecting these overloads in release builds. If a caller passes the wrong UndoLog::Type, this still records a malformed undo log and can break rollback/index cleanup later. Prefer dedicated APIs or a real runtime check here.
Suggested hardening
`#include` <atomic>
-#include <cassert>
`#include` <mutex>
`#include` <optional>
+#include <stdexcept>
`#include` <unordered_set>
`#include` <vector>
@@
void add_undo_log(UndoLog::Type type, const std::string& table_name,
const storage::HeapTable::TupleId& rid) {
- /* Enforce invariant: non-UPDATE types should not provide old_rid through this overload */
- assert(type != UndoLog::Type::UPDATE);
+ if (type == UndoLog::Type::UPDATE) {
+ throw std::invalid_argument("UPDATE undo logs require old_rid");
+ }
undo_logs_.push_back({type, table_name, rid, std::nullopt});
}
@@
void add_undo_log(UndoLog::Type type, const std::string& table_name,
const storage::HeapTable::TupleId& rid,
const storage::HeapTable::TupleId& old_rid) {
- /* Enforce invariant: this overload is primarily for UPDATE types providing old_rid */
- assert(type == UndoLog::Type::UPDATE);
+ if (type != UndoLog::Type::UPDATE) {
+ throw std::invalid_argument("old_rid is only valid for UPDATE undo logs");
+ }
undo_logs_.push_back({type, table_name, rid, old_rid});
}Also applies to: 124-129 and 131-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/transaction/transaction.hpp` around lines 10 - 12, The code currently
uses debug-only asserts to validate UndoLog::Type in the overloads that record
undo-log entries, which allows malformed undo entries in release builds; replace
those assert(...) checks with runtime validation: explicitly check the provided
UndoLog::Type in the overloads that accept it, and if invalid either throw a
descriptive exception or return an error status (do not rely on asserts), and
consider adding dedicated API wrappers (e.g., type-specific record_* methods) so
callers cannot pass the wrong type; update the undo-log write paths (the
overloads that record undo entries) to refuse/handle invalid types and include
clear error messages to prevent corrupt rollback/index cleanup.
Comment on lines
542
to
+557
| if (table.remove(rid, xmax)) { | ||
| /* Update Indexes */ | ||
| std::string err; | ||
| if (!old_tuple.empty()) { | ||
| for (const auto& idx_info : table_meta->indexes) { | ||
| if (!idx_info.column_positions.empty()) { | ||
| uint16_t pos = idx_info.column_positions[0]; | ||
| common::ValueType ktype = table_meta->columns[pos].type; | ||
| storage::BTreeIndex index(idx_info.name, bpm_, ktype); | ||
| if (!apply_index_write(index, old_tuple.get(pos), rid, IndexOp::Remove, | ||
| err)) { | ||
| throw std::runtime_error(err); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if UndoLog tracks index operations
rg -n "UndoLog" --type cpp -C3 | head -60Repository: poyrazK/cloudSQL
Length of output: 4214
🏁 Script executed:
cat -n src/executor/query_executor.cpp | sed -n '534,572p'Repository: poyrazK/cloudSQL
Length of output: 2053
🏁 Script executed:
ast-grep --pattern 'void apply_index_write($_, $_, $_, $_, $_)' src/executor/query_executor.cppRepository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
rg -n "apply_index_write" --type cpp -A15 | head -80Repository: poyrazK/cloudSQL
Length of output: 5804
🏁 Script executed:
cat -n src/transaction/transaction_manager.cpp | sed -n '200,240p'Repository: poyrazK/cloudSQL
Length of output: 2473
🏁 Script executed:
rg -n "rollback\|abort" --type cpp src/transaction/transaction_manager.cpp -i | head -30Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
rg -n "execute_abort\|rollback" --type cpp src/executor/query_executor.cpp -i | head -20Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
rg -n "void.*rollback\|void.*undo" --type cpp src/transaction/ -A3Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
rg -n "~Transaction\|destructor" --type cpp include/transaction/Repository: poyrazK/cloudSQL
Length of output: 42
🏁 Script executed:
rg -n "class Transaction" --type cpp -A30 include/transaction/transaction.hppRepository: poyrazK/cloudSQL
Length of output: 2327
🏁 Script executed:
rg -n "undo_logs_\|rollback\|cleanup" --type cpp src/transaction/transaction_manager.cpp | head -40Repository: poyrazK/cloudSQL
Length of output: 42
Critical: Partial failure window before undo log creation in DELETE, INSERT, and UPDATE operations.
The code marks the heap as modified (via table.remove() on line 542) before adding the undo log (line 569). If apply_index_write() throws an exception (line 553), the undo log is never created, preventing proper rollback. The tuple remains deleted in the heap, but the transaction has no undo record to restore it.
This same vulnerability exists in:
- INSERT operation (around line 446): undo_log added after index operations
- UPDATE operation (around line 639): undo_log added after index operations
Fix by either:
- Adding undo log immediately after heap modification, before index operations
- Moving all operations into a transactional context that atomically succeeds or fails
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executor/query_executor.cpp` around lines 542 - 557, The heap is modified
by table.remove(...) before the undo log is created, so an exception from
apply_index_write(...) (e.g., during the loop over table_meta->indexes or
IndexOp::Remove) leaves the heap changed with no undo record; fix by creating
and appending the undo log immediately after the successful table.remove(rid,
xmax) and before any index operations (refer to table.remove, old_tuple,
apply_index_write, IndexOp::Remove and the undo_log creation site), and make the
same change for INSERT and UPDATE code paths (ensure undo_log is added right
after the heap/tuple change and prior to any index writes or anything that can
throw, or alternatively wrap the heap change+index updates+undo append in a
transactional/RAII scope that rolls back on exceptions).
Comment on lines
630
to
+668
| for (const auto& op : updates) { | ||
| /* Retrieve old tuple for logging */ | ||
| Tuple old_tuple; | ||
| if (log_manager_ != nullptr && txn != nullptr) { | ||
| static_cast<void>(table.get(op.rid, old_tuple)); | ||
| } | ||
|
|
||
| if (table.remove(op.rid, txn_id)) { | ||
| /* Update Indexes - Remove old, Insert new */ | ||
| std::string err; | ||
| for (const auto& idx_info : table_meta->indexes) { | ||
| if (!idx_info.column_positions.empty()) { | ||
| uint16_t pos = idx_info.column_positions[0]; | ||
| common::ValueType ktype = table_meta->columns[pos].type; | ||
| storage::BTreeIndex index(idx_info.name, bpm_, ktype); | ||
| if (!apply_index_write(index, op.old_tuple.get(pos), op.rid, IndexOp::Remove, | ||
| err)) { | ||
| throw std::runtime_error(err); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* Log DELETE part of update */ | ||
| if (log_manager_ != nullptr && txn != nullptr) { | ||
| recovery::LogRecord log(txn->get_id(), txn->get_prev_lsn(), | ||
| recovery::LogRecordType::MARK_DELETE, table_name, op.rid, | ||
| old_tuple); | ||
| op.old_tuple); | ||
| const auto lsn = log_manager_->append_log_record(log); | ||
| txn->set_prev_lsn(lsn); | ||
| } | ||
|
|
||
| const auto new_tid = table.insert(op.new_tuple, txn_id); | ||
|
|
||
| /* Update Indexes - Insert new */ | ||
| for (const auto& idx_info : table_meta->indexes) { | ||
| if (!idx_info.column_positions.empty()) { | ||
| uint16_t pos = idx_info.column_positions[0]; | ||
| common::ValueType ktype = table_meta->columns[pos].type; | ||
| storage::BTreeIndex index(idx_info.name, bpm_, ktype); | ||
| if (!apply_index_write(index, op.new_tuple.get(pos), new_tid, IndexOp::Insert, | ||
| err)) { | ||
| throw std::runtime_error(err); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same partial failure risk as DELETE: index operations not transactionally safe.
The UPDATE path has the same issue—if index operations fail after heap modifications succeed, the system can end up in an inconsistent state where:
- Old index entries point to deleted tuples, or
- New tuples have no corresponding index entries
This compounds the concern raised for DELETE above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executor/query_executor.cpp` around lines 630 - 668, The UPDATE path
performs heap remove/insert and then index writes (using table.remove,
table.insert and apply_index_write) which can leave heap and indexes
inconsistent if an index write fails; make the sequence transactional by either
(A) performing index updates via the same transactional/index API before making
the heap change, or (B) wrapping heap+index operations in a try/catch and
rolling back heap changes on failure: if apply_index_write fails after
table.remove, re-insert op.old_tuple at op.rid (or delete the newly inserted
new_tid) and undo any index changes already applied, then propagate the error;
ensure log_manager_ / txn LSN updates (LogRecord MARK_DELETE and
txn->set_prev_lsn) are only committed after all index writes succeed, and prefer
using txn-aware index operations so apply_index_write is atomic with the
transaction.
Comment on lines
+874
to
878
| /* 3. Filter (WHERE) - Only if not already handled by IndexScan */ | ||
| if (stmt.where() && !index_used) { | ||
| current_root = | ||
| std::make_unique<FilterOperator>(std::move(current_root), stmt.where()->clone()); | ||
| } |
There was a problem hiding this comment.
Filter incorrectly skipped for compound WHERE clauses.
When index_used is true, the entire WHERE filter is skipped. However, the index scan only handles the single equality predicate used for index lookup. If the WHERE clause contains additional conditions (e.g., WHERE id = 3 AND status = 'active'), those extra predicates will be silently ignored.
Consider applying a residual filter for any remaining predicates not satisfied by the index scan.
Sketch of potential fix
- /* 3. Filter (WHERE) - Only if not already handled by IndexScan */
- if (stmt.where() && !index_used) {
+ /* 3. Filter (WHERE) - Skip only simple equality if handled by IndexScan */
+ if (stmt.where()) {
+ /* TODO: For compound conditions, extract residual predicates not
+ * covered by the index scan and apply them here. For now, only
+ * skip if the entire WHERE is the indexed equality. */
+ bool entire_where_handled = index_used &&
+ stmt.where()->type() == parser::ExprType::Binary;
+ if (!entire_where_handled) {
current_root =
std::make_unique<FilterOperator>(std::move(current_root), stmt.where()->clone());
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/executor/query_executor.cpp` around lines 874 - 878, The current code
skips adding a FilterOperator whenever index_used is true, which drops
non-indexed predicates; instead, after creating the IndexScan (when index_used
== true) compute the residual predicate = original stmt.where() minus the
equality predicate consumed by the IndexScan (the predicate the IndexScan used
for lookup), and if that residual is non-empty wrap current_root with
std::make_unique<FilterOperator>(std::move(current_root), residual->clone()).
Use the same symbols shown (stmt.where(), index_used, current_root,
FilterOperator, IndexScan) — ensure IndexScan exposes or returns the consumed
predicate so you can subtract it from stmt.where() and only apply the remaining
predicates as the residual filter.
Comment on lines
129
to
149
| if (consume(TokenType::Join)) { | ||
| join_type = SelectStatement::JoinType::Inner; | ||
| } else if (consume(TokenType::Left)) { | ||
| static_cast<void>(consume(TokenType::Outer)); | ||
| if (!consume(TokenType::Join)) { | ||
| return nullptr; | ||
| } | ||
| join_type = SelectStatement::JoinType::Left; | ||
| } else if (consume(TokenType::Right)) { | ||
| static_cast<void>(consume(TokenType::Outer)); | ||
| if (!consume(TokenType::Join)) { | ||
| return nullptr; | ||
| } | ||
| join_type = SelectStatement::JoinType::Right; | ||
| } else if (consume(TokenType::Full)) { | ||
| static_cast<void>(consume(TokenType::Outer)); | ||
| if (!consume(TokenType::Join)) { | ||
| return nullptr; | ||
| } | ||
| join_type = SelectStatement::JoinType::Full; | ||
| } else { |
There was a problem hiding this comment.
Handle explicit INNER JOIN here as well.
Line 129 only accepts bare JOIN, while the new branches now recognize LEFT, RIGHT, and FULL. SELECT ... INNER JOIN ... still won't parse through this loop, so a common SQL form remains unsupported even though TokenType::Inner exists.
Suggested fix
SelectStatement::JoinType join_type = SelectStatement::JoinType::Inner;
if (consume(TokenType::Join)) {
join_type = SelectStatement::JoinType::Inner;
+ } else if (consume(TokenType::Inner)) {
+ if (!consume(TokenType::Join)) {
+ return nullptr;
+ }
+ join_type = SelectStatement::JoinType::Inner;
} else if (consume(TokenType::Left)) {
static_cast<void>(consume(TokenType::Outer));
if (!consume(TokenType::Join)) {
return nullptr;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (consume(TokenType::Join)) { | |
| join_type = SelectStatement::JoinType::Inner; | |
| } else if (consume(TokenType::Left)) { | |
| static_cast<void>(consume(TokenType::Outer)); | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Left; | |
| } else if (consume(TokenType::Right)) { | |
| static_cast<void>(consume(TokenType::Outer)); | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Right; | |
| } else if (consume(TokenType::Full)) { | |
| static_cast<void>(consume(TokenType::Outer)); | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Full; | |
| } else { | |
| if (consume(TokenType::Join)) { | |
| join_type = SelectStatement::JoinType::Inner; | |
| } else if (consume(TokenType::Inner)) { | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Inner; | |
| } else if (consume(TokenType::Left)) { | |
| static_cast<void>(consume(TokenType::Outer)); | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Left; | |
| } else if (consume(TokenType::Right)) { | |
| static_cast<void>(consume(TokenType::Outer)); | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Right; | |
| } else if (consume(TokenType::Full)) { | |
| static_cast<void>(consume(TokenType::Outer)); | |
| if (!consume(TokenType::Join)) { | |
| return nullptr; | |
| } | |
| join_type = SelectStatement::JoinType::Full; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parser/parser.cpp` around lines 129 - 149, The parser accepts bare JOIN
and LEFT/RIGHT/FULL but not the explicit INNER token, so add a branch that
handles consume(TokenType::Inner) similar to the existing branches: optionally
consume TokenType::Outer (if you want to mirror the other branches), then
require consume(TokenType::Join) and set join_type =
SelectStatement::JoinType::Inner; update the chain around the other consume(...)
branches in parser.cpp (the block using consume and SelectStatement::JoinType)
to include this TokenType::Inner branch so "INNER JOIN" parses the same as bare
"JOIN".
Comment on lines
+254
to
+270
| if (consume(TokenType::Limit)) { | ||
| if (limit_set) return nullptr; | ||
| const Token val = next_token(); | ||
| if (val.type() == TokenType::Number) { | ||
| stmt->set_limit(val.as_int64()); | ||
| limit_set = true; | ||
| } else { | ||
| return nullptr; | ||
| } | ||
| } else if (consume(TokenType::Offset)) { | ||
| if (offset_set) return nullptr; | ||
| const Token val = next_token(); | ||
| if (val.type() == TokenType::Number) { | ||
| stmt->set_offset(val.as_int64()); | ||
| offset_set = true; | ||
| } else { | ||
| return nullptr; |
There was a problem hiding this comment.
Reject decimal literals for LIMIT and OFFSET.
Lines 257-258 and Lines 266-267 accept any TokenType::Number, but this parser also uses TokenType::Number for decimals in parse_primary() (Lines 754-759). That means LIMIT 1.5 / OFFSET 1.5 can be coerced through as_int64() instead of being rejected as invalid SQL.
Suggested fix
if (consume(TokenType::Limit)) {
if (limit_set) return nullptr;
const Token val = next_token();
- if (val.type() == TokenType::Number) {
+ if (val.type() == TokenType::Number &&
+ val.lexeme().find('.') == std::string::npos) {
stmt->set_limit(val.as_int64());
limit_set = true;
} else {
return nullptr;
}
} else if (consume(TokenType::Offset)) {
if (offset_set) return nullptr;
const Token val = next_token();
- if (val.type() == TokenType::Number) {
+ if (val.type() == TokenType::Number &&
+ val.lexeme().find('.') == std::string::npos) {
stmt->set_offset(val.as_int64());
offset_set = true;
} else {
return nullptr;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/parser/parser.cpp` around lines 254 - 270, The LIMIT/OFFSET parsing
currently accepts any TokenType::Number and then calls Token::as_int64(),
allowing decimals like 1.5; update the parsing branch that handles
consume(TokenType::Limit) and consume(TokenType::Offset) to verify the numeric
token is an integer (not a decimal) before calling next_token() -> as_int64();
if the token is a decimal (or otherwise not an integer), return nullptr instead
of coercing, ensuring stmt->set_limit and stmt->set_offset only get integer
values.
Comment on lines
+160
to
+162
| bool TransactionManager::undo_transaction(Transaction* txn) { | ||
| const auto& logs = txn->get_undo_logs(); | ||
| bool success = true; |
There was a problem hiding this comment.
Propagate rollback failure back into abort().
undo_transaction() now surfaces failure, but Line 123 still ignores the returned status. That lets abort() append an ABORT record, release locks, and move the transaction to completed_transactions_ even when rollback left heap/index state inconsistent.
Also applies to: 287-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/transaction/transaction_manager.cpp` around lines 160 - 162,
undo_transaction() now returns a bool on rollback success, but callers (notably
abort()) ignore it; update abort() to check the return value from
TransactionManager::undo_transaction(Transaction*) and if it returns false, stop
the abort path: do not append an ABORT record, do not release locks or push the
txn into completed_transactions_, and instead propagate failure (return false /
error) so higher-level code knows rollback failed; also find the other caller
that invokes undo_transaction (the other site reported) and apply the same
check/propagation so rollback failures aren't masked.
Comment on lines
+187
to
+201
| if (table.get(log.rid, tuple)) { | ||
| for (const auto& idx_info : table_meta->indexes) { | ||
| if (!idx_info.column_positions.empty()) { | ||
| uint16_t pos = idx_info.column_positions[0]; | ||
| common::ValueType ktype = table_meta->columns[pos].type; | ||
| storage::BTreeIndex index(idx_info.name, bpm_, ktype); | ||
| if (!index.remove(tuple.get(pos), log.rid)) { | ||
| std::cerr << "Rollback ERROR: Index remove failed for table '" | ||
| << log.table_name << "', index '" << idx_info.name | ||
| << "'\n"; | ||
| success = false; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Treat table.get() failure as a rollback error.
At Line 187, Line 215, Line 237, and Line 265, a failed table.get(...) just skips index maintenance. In the INSERT and UPDATE-new-version paths, rollback then still calls physical_remove, which can leave dangling index entries; in the DELETE and UPDATE-old-version paths, the row can be restored without rebuilding its indexes. These branches should flip success = false and avoid continuing with heap mutation when the tuple needed for index maintenance cannot be read.
Also applies to: 214-229, 237-251, 264-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/transaction/transaction_manager.cpp` around lines 187 - 201, The rollback
currently ignores table.get(...) failures and proceeds with heap changes,
risking dangling index entries; update each branch that calls table.get(log.rid,
tuple) (the blocks handling INSERT, UPDATE-new-version, DELETE,
UPDATE-old-version) so that if table.get(...) returns false you set success =
false and do not perform any subsequent heap mutation (e.g., do not call
physical_remove or restore the row) for that log entry; specifically, after each
failed table.get(log.rid, tuple) add success = false and bail out of that branch
(skip index maintenance and any physical_remove/restore steps) so the rollback
treats the missing tuple as an error.
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
1 participant
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.
This follow-up PR further stabilizes the execution engine and expands test coverage:
HashJoinOperatorwith proper matched-row tracking.UPDATEandDELETEoperations, utilizing a newly expandedUndoLogandHeapTablerollback primitives.QueryExecutorper connection, enabling multi-statement transactions and automatic rollback on disconnect.LIMIT 0andOFFSETedge case handling across the parser and executor.rowsort/valuesortsupport and verbose failure reporting.Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation