Skip to content

Conversation

@mcllerena
Copy link
Contributor

@mcllerena mcllerena commented Dec 2, 2025

We can check on this approach to better handle band, date_from and date_to, etc

@mcllerena mcllerena requested review from Copilot and pesap December 2, 2025 22:20
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 94.59459% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.39%. Comparing base (51e3f3b) to head (e1efacc).

Files with missing lines Patch % Lines
src/plexosdb/db.py 75.00% 1 Missing ⚠️
src/plexosdb/utils.py 96.96% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   95.15%   95.39%   +0.23%     
==========================================
  Files           8        8              
  Lines        1465     1497      +32     
==========================================
+ Hits         1394     1428      +34     
+ Misses         71       69       -2     

☔ 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.

@github-actions github-actions bot added the tests label Dec 2, 2025
@mcllerena mcllerena changed the title Handle property related attributes on "add_properties_from_records" method fix: handle property related attributes on "add_properties_from_records" method Dec 2, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the property insertion functionality in plexosdb to handle property-related metadata attributes (band, date_from, date_to) directly from record dictionaries. Previously, these attributes could only be added individually; now they can be bulk-inserted alongside properties.

Key changes:

  • Modified prepare_properties_params to extract and return metadata (band, date_from, date_to) from records
  • Updated insert_property_data to accept metadata and persist band/date information to the database
  • Enhanced add_object to support skipping system membership creation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
src/plexosdb/utils.py Extended prepare_properties_params to return metadata_map and updated insert_property_data to handle band/date metadata insertion
src/plexosdb/db.py Added Literal[False] to collection_enum parameter type and updated add_properties_from_records to pass metadata to property insertion
tests/test_utils_properties.py Updated all test cases to handle the new 3-tuple return value from prepare_properties_params and added test with band/date metadata
tests/test_utils_build_data_id_map.py Updated all test cases to destructure the new 3-tuple return value from prepare_properties_params
Comments suppressed due to low confidence (1)

tests/test_utils_properties.py:445

  • The test adds band, date_from, and date_to to the record but doesn't verify that these values are actually persisted to the database. Consider adding assertions to check that:
  1. The band is inserted into t_band table
  2. The dates are inserted into t_date_from and t_date_to tables

For example:

# Verify band was added
band_records = db_with_topology.query("SELECT band_id, data_id FROM t_band")
assert len(band_records) > 0

# Verify dates were added
date_from_records = db_with_topology.query("SELECT data_id, date FROM t_date_from")
assert len(date_from_records) > 0
date_to_records = db_with_topology.query("SELECT data_id, date FROM t_date_to")
assert len(date_to_records) > 0
    records = [
        {
            "name": "gen-01", "Max Capacity": 100.0, "band": 1,
            "date_from": datetime(2025, 1, 1), "date_to": datetime(2025, 12, 31),
        }
    ]

    params, _, metadata_map = prepare_properties_params(
        db_with_topology,
        records,
        ClassEnum.Generator,
        CollectionEnum.Generators,
        ClassEnum.System,
    )

    with db_with_topology._db.transaction():
        insert_property_data(db_with_topology, params, metadata_map)
        # Large chunksize - should process all in one batch
        insert_scenario_tags(db_with_topology, "BatchTest", params, chunksize=1000)

        # Verify tags were inserted
        tag_count = db_with_topology.query("SELECT COUNT(*) FROM t_tag")
        assert tag_count[0][0] > 0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mcllerena mcllerena requested a review from pesap December 4, 2025 17:52
@mcllerena mcllerena merged commit 1776d2a into main Dec 4, 2025
24 checks passed
@pesap pesap deleted the ml/fix-bugs branch December 4, 2025 20:21
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.

4 participants