Conversation
📝 WalkthroughWalkthroughDatabase synchronization logic optimized through set-based lookups replacing O(n) index checks and vectorized column-change detection replacing loop-based comparisons. Hash-based change detection narrows per-cell comparisons to modified rows only, reducing computational overhead while preserving existing validation and diff structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ferry/database/sync_db_courses.py`:
- Around line 120-124: The per-cell diff currently lets pd.NA propagate (via
subset_old == subset_new) so unequal_mask gets <NA> values; change the logic in
the block that computes subset_unequal (using subset_old, subset_new,
possibly_changed) to explicitly handle nulls—for example compute na_mismatch =
subset_old.isna() ^ subset_new.isna(), compute value_diff = (subset_old !=
subset_new).fillna(False), then set subset_unequal = value_diff | na_mismatch
and assign that into unequal_mask.loc[possibly_changed]; this ensures NA→value
and value→NA transitions are detected and yields a boolean mask.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9311fcf0-f37f-48b0-bf69-df641e6460dd
📒 Files selected for processing (1)
ferry/database/sync_db_courses.py
| subset_unequal = ~( | ||
| (subset_old == subset_new) | ||
| | (subset_old.isna() & subset_new.isna()) | ||
| ) | ||
| print(f"Old type: {old_types.iat[row, col]}") | ||
| print(f"New type: {new_types.iat[row, col]}") | ||
| print(f"Old value: {shared_rows_old.iat[row, col]}") | ||
| print(f"New value: {shared_rows_new.iat[row, col]}") | ||
| raise TypeError("Type mismatch") | ||
| unequal_mask = ~( | ||
| (shared_rows_old == shared_rows_new) | ||
| | (shared_rows_old.isna() & shared_rows_new.isna()) | ||
| ) | ||
| unequal_mask.loc[possibly_changed] = subset_unequal |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import pandas as pd
old = pd.DataFrame({"primary_crn": pd.Series([pd.NA], dtype="Int64")})
new = pd.DataFrame({"primary_crn": pd.Series([12345], dtype="Int64")})
current = ~((old == new) | (old.isna() & new.isna()))
fixed = old.ne(new).fillna(True) & ~(old.isna() & new.isna())
print("current mask:")
print(current)
print("current any:", current.any(axis=1).tolist())
print("\nfixed mask:")
print(fixed)
print("fixed any:", fixed.any(axis=1).tolist())
PYRepository: coursetable/ferry
Length of output: 179
🏁 Script executed:
#!/bin/bash
pip install pandas --quiet
python - <<'PY'
import pandas as pd
old = pd.DataFrame({"primary_crn": pd.Series([pd.NA], dtype="Int64")})
new = pd.DataFrame({"primary_crn": pd.Series([12345], dtype="Int64")})
current = ~((old == new) | (old.isna() & new.isna()))
fixed = old.ne(new).fillna(True) & ~(old.isna() & new.isna())
print("current mask:")
print(current)
print("current any:", current.any(axis=1).tolist())
print("\nfixed mask:")
print(fixed)
print("fixed any:", fixed.any(axis=1).tolist())
PYRepository: coursetable/ferry
Length of output: 431
🏁 Script executed:
cd /repo && cat -n ferry/database/sync_db_courses.py | sed -n '100,135p'Repository: coursetable/ferry
Length of output: 116
🏁 Script executed:
cd /repo && cat -n ferry/database/sync_db_courses.py | sed -n '680,695p'Repository: coursetable/ferry
Length of output: 116
🏁 Script executed:
find . -name "sync_db_courses.py" -type fRepository: coursetable/ferry
Length of output: 96
🏁 Script executed:
find . -name "sync_db_courses.py" -type f -exec cat -n {} \; | sed -n '100,135p'Repository: coursetable/ferry
Length of output: 2108
🏁 Script executed:
find . -name "sync_db_courses.py" -type f -exec cat -n {} \; | sed -n '680,695p'Repository: coursetable/ferry
Length of output: 925
Make the per-cell diff mask NA-safe.
With pandas nullable dtypes, subset_old == subset_new returns pd.NA for null/value transitions. That propagates through the boolean expression, causing unequal_mask to contain <NA> cells. When unequal_mask.any(axis=1) is called at line 126, those NA values are treated as falsy, so rows with changes like primary_crn: <NA> → 12345 are silently dropped from the diff.
Suggested fix
- subset_unequal = ~(
- (subset_old == subset_new)
- | (subset_old.isna() & subset_new.isna())
- )
+ both_na = subset_old.isna() & subset_new.isna()
+ subset_unequal = subset_old.ne(subset_new).fillna(True) & ~both_na📝 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.
| subset_unequal = ~( | |
| (subset_old == subset_new) | |
| | (subset_old.isna() & subset_new.isna()) | |
| ) | |
| print(f"Old type: {old_types.iat[row, col]}") | |
| print(f"New type: {new_types.iat[row, col]}") | |
| print(f"Old value: {shared_rows_old.iat[row, col]}") | |
| print(f"New value: {shared_rows_new.iat[row, col]}") | |
| raise TypeError("Type mismatch") | |
| unequal_mask = ~( | |
| (shared_rows_old == shared_rows_new) | |
| | (shared_rows_old.isna() & shared_rows_new.isna()) | |
| ) | |
| unequal_mask.loc[possibly_changed] = subset_unequal | |
| both_na = subset_old.isna() & subset_new.isna() | |
| subset_unequal = subset_old.ne(subset_new).fillna(True) & ~both_na | |
| unequal_mask.loc[possibly_changed] = subset_unequal |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ferry/database/sync_db_courses.py` around lines 120 - 124, The per-cell diff
currently lets pd.NA propagate (via subset_old == subset_new) so unequal_mask
gets <NA> values; change the logic in the block that computes subset_unequal
(using subset_old, subset_new, possibly_changed) to explicitly handle nulls—for
example compute na_mismatch = subset_old.isna() ^ subset_new.isna(), compute
value_diff = (subset_old != subset_new).fillna(False), then set subset_unequal =
value_diff | na_mismatch and assign that into
unequal_mask.loc[possibly_changed]; this ensures NA→value and value→NA
transitions are detected and yields a boolean mask.
1 participant
Summary by CodeRabbit