Skip to content

fix: resolve bug on add properties from records related methods#129

Open
mcllerena wants to merge 4 commits intomainfrom
ml/fix-bug
Open

fix: resolve bug on add properties from records related methods#129
mcllerena wants to merge 4 commits intomainfrom
ml/fix-bug

Conversation

@mcllerena
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added the tests label Apr 29, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.66%. Comparing base (c5d9a38) to head (41a380b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   95.64%   95.66%   +0.02%     
==========================================
  Files           8        8              
  Lines        1721     1729       +8     
==========================================
+ Hits         1646     1654       +8     
  Misses         75       75              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/plexosdb/utils.py
Comment on lines +500 to +503
param_tuple = (membership_id, property_id, record["value"])
row_key = (membership_id, property_id, record["value"], row_index)
params.append(param_tuple)
metadata_map[row_key] = {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Index source can drift here. metadata_map keys use row_index from normalized_records, but downstream maps and lookups key off enumerate(params). If a row is skipped before append, metadata can miss on later valid rows.

Suggested change:

Suggested change
param_tuple = (membership_id, property_id, record["value"])
row_key = (membership_id, property_id, record["value"], row_index)
params.append(param_tuple)
metadata_map[row_key] = {
insert_index = len(params)
param_tuple = (membership_id, property_id, record["value"])
row_key = (membership_id, property_id, record["value"], insert_index)
params.append(param_tuple)
metadata_map[row_key] = {

Then use the same insert_index convention in later key lookups too (insert_property_values, apply_scenario_tags, _collect_text_rows).

assert len(rows) == 2
assert {row[1] for row in rows} == {1, 2}
assert {row[2] for row in rows} == {1}
assert {row[3] for row in rows} == {"row1.csv", "row2.csv"}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great coverage for duplicate value rows. Please add one regression with a skipped row before valid rows to lock the index alignment fix.

Suggested change:

Suggested change
assert {row[3] for row in rows} == {"row1.csv", "row2.csv"}
assert {row[3] for row in rows} == {"row1.csv", "row2.csv"}
def test_add_properties_from_records_duplicate_rows_after_skipped_row_preserve_links(
db_instance_with_schema: PlexosDB,
) -> None:
from plexosdb import ClassEnum, CollectionEnum
db = db_instance_with_schema
db.add_object(ClassEnum.Generator, "GEN_MIX")
records = [
{
"name": "MISSING_OBJECT",
"property": "Max Capacity",
"value": 0.0,
"band": 9,
"datafile_text": "skip.csv",
},
{
"name": "GEN_MIX",
"property": "Max Capacity",
"value": 0.0,
"band": 1,
"datafile_text": "row1.csv",
},
{
"name": "GEN_MIX",
"property": "Max Capacity",
"value": 0.0,
"band": 2,
"datafile_text": "row2.csv",
},
]
db.add_properties_from_records(
records,
object_class=ClassEnum.Generator,
parent_class=ClassEnum.System,
collection=CollectionEnum.Generators,
scenario="ScenarioMix",
)
rows = db.query(
"""
SELECT d.data_id, b.band_id
FROM t_data d
LEFT JOIN t_band b ON b.data_id = d.data_id
JOIN t_membership m ON d.membership_id = m.membership_id
JOIN t_object o ON m.child_object_id = o.object_id
JOIN t_property p ON d.property_id = p.property_id
WHERE o.name = ? AND p.name = ?
ORDER BY d.data_id
""",
("GEN_MIX", "Max Capacity"),
)
assert len(rows) == 2
assert {row[1] for row in rows} == {1, 2}

Copy link
Copy Markdown
Collaborator

@pesap pesap left a comment

Choose a reason for hiding this comment

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

Left two inline comments with concrete suggestions. Main blocker is row index key drift between normalized_records and params, which can drop metadata/text/tag links after skipped rows. Please fix index source and add a mixed valid plus skipped regression test.

@mcllerena mcllerena requested a review from pesap April 30, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: add_properties_from_records silently drops band/date metadata for records sharing (name, property, value)

3 participants