Skip to content

Resolve column renames on the ATTACH channel import path - #15142

Merged
rtibbles merged 3 commits into
learningequality:developfrom
rtibblesbot:issue-15098-9ca749
Aug 4, 2026
Merged

Resolve column renames on the ATTACH channel import path#15142
rtibbles merged 3 commits into
learningequality:developfrom
rtibblesbot:issue-15098-9ca749

Conversation

@rtibblesbot

@rtibblesbot rtibblesbot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Importing a channel published before schema 6 transferred content_localfile a row at a time through a Python mapper, because the destination's file_size_bigint column reads from a differently-named source column and that disqualified the whole table from the single-statement ATTACH path. The ATTACH guard now accepts a per_row mapping whose value names a column on the source table, and the generated SELECT reads that column directly.

Only LocalFile newly qualifies, under NoIncludedPresetsChannelImport and the NoLearningActivitiesChannelImport that inherits its entry. The three other renames named in the issue stay on the row path for reasons this change does not touch: File.local_file_id and ChannelMetadata.root_id sit beside a "post" key and a callable mapper respectively, and LocalFile under NoVersionChannelImport carries a per_table mapper.

References

Fixes #15098. Benchmark harness: #15110.

Reviewer guidance

  • channel_import.py:482 — the resolver prefers a source column over a same-named class attribute, mirroring generate_row_mapper:405-426. Check the two orders cannot drift apart, since a disagreement would silently insert a different value on each path.
  • channel_import.py:724can_use_sqlite_attach_method has a second caller in _can_use_optimized_pre_deletion, where a True makes delete_old_channel_tree_data skip rows. LocalFile is filtered out of models_to_delete by merge_models, so it cannot reach that caller today; check that argument still holds if another model later qualifies.
  • sqlite_table_import truncates strings to max_length and substitutes column defaults for None, and the ATTACH path does neither. Confirm the schema 5 content_localfile columns admit no difference — extension is String(40) on both sides, available is a constant under both paths, and file_size_bigint is nullable with no default.

Measured gain

Two captures of integration_testing/scripts/content_operations_benchmark.py, one harness copy, same machine and KOLIBRI_HOME, --runs 3, one sitting, against c9d7f950ab6b5a1199e3d6c10d7f0103 (Khan Academy English US, 34,488 content nodes, 77,315 local files).

Phase Baseline Branch Diff
fresh_import 12.839s 12.210s −4.9%
fresh_import_no_attach 13.108s 13.059s −0.4%
annotation 2.457s 2.473s +0.7%
upgrade_import 18.018s 17.445s −3.2%

Within the branch capture, which shares a sitting and a preceding file copy and so beats the cross-capture delta, fresh_import comes out at 12.210s against fresh_import_no_attach at 13.059s.

The statement counts carry more than the times: fresh_import drops from 405 SQLAlchemy statements to 327. That −78 is exactly the row path's 78 batched inserts over 77,315 local files, plus the source SELECT they were read from, collapsing into one INSERT OR IGNORE … SELECT. annotation and drive_enumeration are flat, so no capture-wide contention is in play.

Acceptance criteria

Ticked here rather than on the issue: this account cannot edit the issue body.

  • A table whose only per_row mappings are constants or renames uses the ATTACH path.
  • The generated SELECT reads the renamed destination column from its source column.
  • A table with a callable mapper or a per_table mapper still uses the row-by-row path.
  • A mapping value matching neither a source column nor a class attribute still raises, rather than silently inserting a default.
  • content_localfile imports via ATTACH for a schema 5 channel.
  • A test asserts can_use_sqlite_attach_method returns True for a rename mapping and False for a callable mapper.
  • Imports of all nine schema versions produce identical row data before and after, with test_channel_import.py green — 289 passed on both SQLite and PostgreSQL.
  • file_size_bigint values imported via ATTACH match those imported via the row path for the same source.

AI usage

Used Claude Code to relax the guard, write the tests and take both benchmark captures, following a plan whose correctness argument I directed — in particular the NULL-parity and column-compatibility checks that make moving this one table safe. Verified with the channel import suite on SQLite and PostgreSQL, and with the content operations benchmark.


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?
  • Ran pre-flight CI checks (lint, format, tests) and verified all pass
  • Rebased onto the target branch and resolved any conflicts
  • Reorganized commit history into clean, logical commits
  • Audited the diff to ensure only issue-relevant files are changed
  • Built PR body from the repository's PR template with evidence blocks
@rtibblesbot

🟡 Waiting for feedback

Last updated: 2026-08-04 09:28 UTC

@github-actions github-actions Bot added DEV: backend Python, databases, networking, filesystem... SIZE: medium labels Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

@rtibblesbot
rtibblesbot marked this pull request as ready for review August 4, 2026 08:47
rtibblesbot and others added 3 commits August 4, 2026 02:18
A per_row mapping whose value names a column on the source table is a
rename, and a rename is expressible directly in the generated SELECT.
Both can_use_sqlite_attach_method and raw_attached_sqlite_table_import
previously resolved a mapping only against attributes of the import
class, so any rename disqualified the whole table from the single
statement transfer and sent it down the row-by-row Python mapper.

Give both a shared resolver that mirrors generate_row_mapper's order —
source column first, then non-callable class attribute — returning the
SQL fragment, or None when the mapping has no SQL equivalent. The guard
reads None as reject; the SELECT builder reads it as raise. Callable
mappers and per_table mappers still take the row-by-row path.

Only content_localfile newly qualifies, under the schema 5 and earlier
import classes, via the file_size to file_size_bigint rename.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard unit tests prove the decision; these prove a real schema 5
import acts on it. One asserts content_localfile reaches
raw_attached_sqlite_table_import, the other that the file_size_bigint
values it lands — a set size and four NULLs — match a re-import forced
down the row-by-row path.

Both flush before re-importing: check_and_delete_existing_channel
cancels a same-version re-import, so without an empty database
set_content_fixture imports nothing and both assertions pass vacuously.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment above BENCHMARK_CHANNELS predicted the two import phases would
land near-flat, on the grounds that the mapping class maps ContentNode with
a "post" key. That half still holds, but content_localfile now ATTACHes, so
the phases diverge by roughly that table's cost.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes make perfect sense - no changes to the core channel import tests and they still pass, and the benchmark results show a modest improvement in performance.

The reduction in number of issued queries is the most significant.

@rtibbles
rtibbles merged commit b7ec2bb into learningequality:develop Aug 4, 2026
82 checks passed
@rtibblesbot
rtibblesbot deleted the issue-15098-9ca749 branch August 4, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DEV: backend Python, databases, networking, filesystem... SIZE: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow the ATTACH import path to handle column renames

2 participants