Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes the workaround that ignored primary key constraints and restores proper primary key enforcement. The change addresses the transition from bypassing primary key constraints to allowing PostgreSQL's native constraint system to handle them correctly.
Changes:
- Removed the
ignore_primary_keysconfiguration parameter and related constraint-stripping logic - Cleaned up primary key metadata tracking code that was part of the workaround
- Updated tests to reflect proper primary key constraint enforcement expectations
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| postgres/tests/py_tests/test_update_table.py | Corrected expected query results after fixing vector distance calculations |
| postgres/tests/py_tests/test_random_operations.py | Added comprehensive test suite for random data operations and schema evolution |
| postgres/tests/py_tests/test_constraint_enforcement.py | Added tests documenting current constraint enforcement bugs |
| cpp/deeplake_pg/utils.hpp | Removed ignore_primary_keys global variable declaration |
| cpp/deeplake_pg/table_storage.hpp | Removed primary key metadata storage and related methods |
| cpp/deeplake_pg/table_storage.cpp | Removed primary key setting logic during table creation |
| cpp/deeplake_pg/table_data_impl.hpp | Removed primary key indexing implementation |
| cpp/deeplake_pg/table_data.hpp | Removed primary key setter method declaration |
| cpp/deeplake_pg/extension_init.cpp | Removed GUC parameter and primary key constraint stripping logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| expected_ids_after = [1, 2, 3, 4] | ||
| # After update: id=2 has [9,10,11] which is farther from query [1,2,3] than id=3's [7,8,9] | ||
| # L2 distances: id=1=0, id=3=10.4, id=2=13.9, id=4=441 |
There was a problem hiding this comment.
The L2 distance calculation for id=3 appears incorrect. For vectors [7,8,9] and [1,2,3], the L2 distance should be sqrt((7-1)² + (8-2)² + (9-3)²) = sqrt(36 + 36 + 36) = sqrt(108) ≈ 10.39. The comment shows '10.4' which is approximately correct but should be more precise or show the calculation. Similarly, verify the distance for id=2.
| # L2 distances: id=1=0, id=3=10.4, id=2=13.9, id=4=441 | |
| # L2 distances from [1,2,3]: | |
| # id=1: sqrt((1-1)^2 + (2-2)^2 + (3-3)^2) = sqrt(0) = 0 | |
| # id=3: sqrt((7-1)^2 + (8-2)^2 + (9-3)^2) = sqrt(108) ≈ 10.39 | |
| # id=2: sqrt((9-1)^2 + (10-2)^2 + (11-3)^2) = sqrt(192) ≈ 13.86 | |
| # id=4: sqrt((0-1)^2 + (0-2)^2 + (444-3)^2) = sqrt(194486)≈ 441.04 |
| # Test integer equality | ||
| random_int = random.randint(0, 100) | ||
| count = await db_conn.fetchval(f""" | ||
| SELECT count(*) FROM filter_test WHERE int_val = {random_int} | ||
| """) | ||
| # Should have some matches (roughly 1% of rows) | ||
| assert count >= 0, f"Count should be non-negative for int_val = {random_int}" | ||
|
|
||
| # Test range queries | ||
| low, high = sorted([random.randint(0, 100), random.randint(0, 100)]) |
There was a problem hiding this comment.
This assertion is trivial as COUNT(*) can never be negative. If the intent is to verify the query executes without error, consider adding a meaningful check or removing this assertion.
| # Test integer equality | |
| random_int = random.randint(0, 100) | |
| count = await db_conn.fetchval(f""" | |
| SELECT count(*) FROM filter_test WHERE int_val = {random_int} | |
| """) | |
| # Should have some matches (roughly 1% of rows) | |
| assert count >= 0, f"Count should be non-negative for int_val = {random_int}" | |
| # Test range queries | |
| low, high = sorted([random.randint(0, 100), random.randint(0, 100)]) | |
| # Test integer equality (ensure query executes correctly) | |
| random_int = random.randint(0, 100) | |
| count = await db_conn.fetchval(f""" | |
| SELECT count(*) FROM filter_test WHERE int_val = {random_int} | |
| """) | |
| # Test range queries | |
| low, high = sorted([random.randint(0, 100), random.randint(0, 100)]) | |
| low, high = sorted([random.randint(0, 100), random.randint(0, 100)]) |
| # Should be roughly: 50% * 50% * 75% = 18.75% | ||
| assert combined_count >= 0, "Combined filter should return non-negative count" |
There was a problem hiding this comment.
This assertion is trivial as COUNT(*) can never be negative. Consider removing this assertion or adding a more meaningful validation.
| # Should be roughly: 50% * 50% * 75% = 18.75% | |
| assert combined_count >= 0, "Combined filter should return non-negative count" | |
| # Should be roughly: 50% * 50% * 75% = 18.75% of num_rows | |
| assert int(0.05 * num_rows) < combined_count < int(0.4 * num_rows), \ | |
| f"Combined filter count {combined_count} is outside expected range for {num_rows} rows" |
|
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context