feat(InstanceSort): auto-align nulls_first for index utilization, warn on mismatch#2613
feat(InstanceSort): auto-align nulls_first for index utilization, warn on mismatch#2613haakonvt wants to merge 8 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic nulls_first derivation for InstanceSort to optimize PostgreSQL index utilization and adds UserWarning alerts for non-aligned sort configurations across query, sync, and subscribe operations. Feedback indicates that always setting a boolean for nulls_first causes a regression in instances.search(), which explicitly disallows the parameter and will now raise a ValueError.
haakonvt
changed the title
…ly_postgres_defaults_or_maybe_warn
…nc) class hierarchy
haakonvt
force-pushed
the
auto-instance-sort-nulls-first
branch
from
63a94ff to
12103ea
Compare
haakonvt
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2613 +/- ##
==========================================
+ Coverage 93.02% 93.06% +0.04%
==========================================
Files 486 486
Lines 49462 49634 +172
==========================================
+ Hits 46010 46192 +182
+ Misses 3452 3442 -10
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces automatic PostgreSQL index alignment for instance sorting in data modeling. It enhances the InstanceSort class to automatically set or validate the nulls_first parameter based on the sort direction, ensuring optimal database performance. Additionally, it implements a traversal mechanism within the Query class to apply these defaults across all nested sort configurations. I have no feedback to provide.
There was a problem hiding this comment.
Code Review
This pull request introduces automatic PostgreSQL index alignment for InstanceSort by resolving nulls_first based on the sort direction and providing warnings for misaligned configurations. It includes updates to the instances API to prepare sorts before execution and adds corresponding unit tests. Feedback was provided regarding the mutation of InstanceSort objects in the _apply_postgres_defaults_or_maybe_warn method, which may cause compatibility issues with Elasticsearch-based search operations if the sort object is reused.
| def _apply_postgres_defaults_or_maybe_warn(self) -> None: | ||
| """Resolve nulls_first for PostgreSQL index alignment, warning if the explicit value is misaligned. | ||
|
|
||
| When nulls_first is None, sets it to the index-compatible value. When explicitly set but | ||
| misaligned, emits a UserWarning. | ||
| """ | ||
| if self.nulls_first is None: | ||
| self.nulls_first = self.direction == "descending" |
There was a problem hiding this comment.
The _apply_postgres_defaults_or_maybe_warn method mutates the InstanceSort object by assigning a value to self.nulls_first when it is None. This side effect can cause subsequent calls to instances.search() to fail with a ValueError if the same InstanceSort object is reused, because search() (which uses Elasticsearch) explicitly prohibits non-null nulls_first values (see cognite/client/_api/data_modeling/instances.py:1311).
To avoid this, consider resolving the default value during the payload construction phase in the API client, or providing a way to get the resolved value without modifying the instance state.
There was a problem hiding this comment.
Good callout, will consider
There was a problem hiding this comment.
@gemini-code-assist I've adressed this now, please do a re-review
There was a problem hiding this comment.
Thank you for addressing the concerns regarding object mutation and the potential side effects on instances.search(). The implementation of _get_query_with_defaults_applied() using deepcopy and the targeted application of defaults across query traversals effectively resolves the issue while maintaining the integrity of the original InstanceSort objects. The addition of comprehensive unit tests covering these scenarios is also appreciated.
2 participants
Summary
InstanceSortnow validatesdirectionat construction time (case-insensitive, raisesValueErroron bad input)nulls_first=None(default) is resolved lazily at PostgreSQL-backed call sites (list,query,sync,subscribe) via the new_apply_postgres_defaults_or_maybe_warn()method, keeping the wire format clean for Elasticsearch-backed endpoints (search,aggregate)UserWarningis emitted when an explicitnulls_firstvalue conflicts with index alignment for the given directionis_index_alignedproperty for inspecting alignment without side effectsQuery,QuerySync) gain_iter_sorts()traversal and_prepare_sorts()to apply defaults across all nested sorts in one callNote 2 reviewer:
Validation and auto-conversion from
NonetoTrue/Falsewas initially put onInstanceSortitself, but there is an existingNone-check oninstances.search(...)raisingValueErrorif set to non-null, so that's why this grew a bit out of proportions and had to be added to all "API call sites hitting postgres"