Fixed out of bounds fix after drop column.

[khustup2]

🚀 🚀 Pull Request

Impact

• Bug fix (non-breaking change which fixes expected existing functionality)
• Enhancement/New feature (adds functionality without impacting existing logic)
• Breaking change (fix or feature that would cause existing functionality to change)

Description

Things to be aware of

Things to worry about

Additional Context

[claude]

Claude finished @khustup2's task in 6m 29s —— View job

---

PR Review: Fixed out of bounds fix after drop column

I've reviewed the changes in this PR. The fix correctly addresses the out-of-bounds access issue when columns are dropped by properly mapping between logical column indices and TupleDesc indices.

Summary

Changes look correct overall. The PR:

1. ✅ Introduces proper index mapping via logical_index_for_attnum() and tupdesc_to_logical_
2. ✅ Fixes slot array indexing in table_scan_impl.hpp and table_data_impl.hpp
3. ✅ Fixes index mapping in deeplake_executor.cpp, table_am.cpp, and column_statistics.cpp
4. ✅ Includes an important deadlock fix in table_storage.cpp

Architecture Understanding

The code maintains three index spaces:

• Logical index: 0-based, excludes dropped columns (used internally for DeepLake columns)
• TupleDesc index: 0-based, includes dropped columns (used for PostgreSQL slot arrays)
• attnum: 1-based PostgreSQL attribute number

The mappings:

• active_column_indices_[logical] → TupleDesc index
• tupdesc_to_logical_[tupdesc_idx] → logical index (-1 if dropped)
• get_tupdesc_index(logical) → TupleDesc index
• logical_index_for_attnum(attnum) → logical index

Minor Observations

1. Comment in table_storage.cpp:954-958: Excellent explanation of the deadlock scenario. This is a subtle but critical fix.

2. Consistency: All the changes consistently apply the mapping pattern, which reduces the chance of future bugs.

3. Bounds checking: The logical_index_for_attnum() function properly returns -1 for out-of-range or dropped columns, and all call sites check for this.

No significant issues found. The changes appear to correctly fix the out-of-bounds access issue that would occur when columns are dropped.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Bug: Incorrect slot position mapping in convert_nd_to_pg

The col variable here is a logical column index (0-based, excluding dropped columns), but slot_pos is the TupleDesc index. However, the values and nulls arrays passed to this function are sized according to tupdesc->natts and should be indexed by TupleDesc position, not slot position.

Problem: When a column is dropped, TupleDesc indices don't match logical indices. For example, if column 1 is dropped in a 3-column table:

• Logical index 0 → TupleDesc index 0 (OK)
• Logical index 1 → TupleDesc index 2 (maps to physical column 2, but slot arrays expect index 1)

This causes out-of-bounds writes to values[] and nulls[] arrays when there are dropped columns, corrupting memory.

Fix: The function should directly index values[slot_pos] and nulls[slot_pos] without going through get_tupdesc_index(), since slot_pos already IS the correct TupleDesc index. The current code double-maps the index incorrectly.

Suggested change

	const auto slot_pos = table_data_.get_tupdesc_index(col);
	for (auto col : null_columns_) {
	nulls[col] = true;
	}
	for (auto col : scored_columns_) {
	nulls[col] = false;
	}
	for (auto col : special_columns_) {
	values[col] = pg::utils::make_special_datum(table_id_, row_number, col, table_data_.get_base_atttypid(col));
	nulls[col] = false;
	}
	for (auto col : process_columns_) {
	auto [datum, is_null] = get_datum(col, row_number);
	values[col] = datum;
	nulls[col] = is_null;
	}

Wait, I need to reconsider this...