Performance 12498089633: Make more index key reading async

[alexowens90]

Reference Issues/PRs

12498089633

What does this implement or fix?

read_index_key_without_column_stats called read_sync to get the index key in many async contexts that effectively added a blocking IO to what were meant to be completely asynchronous functions.
This makes read_index_key_without_column_stats async, and chains futures from it where necessary (synchronous callers just call .get() immediately. The sync version read_index_key_without_column_stats_sync has been added for use in merge_update_impl to avoid rebase nightmares with ongoing work, @vasil-pashov will switch it to using the async version and remove the sync version as part of one of his PRs in this area.

[claude]

ArcticDB Code Review Summary

Reviewed the async refactor of read_index_key_without_column_stats (index key reading made async, futures chained through compact_data_impl, compact_data_explain_plan_impl, read_and_process; a _sync variant retained for merge_update_impl).

No blocking issues found in the async refactor. (See previous notes on §10 async/deadlock, executor choice, and lifetimes — all still hold.)

---

Update — new commits (delta reviewed against 4dea3d1..ddb63d9)

The latest commits add a separate, substantial feature: NAN_COUNT_V1 / NULL_COUNT_V1 column stats plus NaN/null-aware query pruning (proto enum additions, MinMaxAggregatorData now counts in-band NaN/NaT and sparse-gap nulls, adjust_for_missing + only_nulls() downgrade prune verdicts in the dispatch layer). This is outside the PR's stated scope ("Make more index key reading async") — the delta looks like a rebase/restack onto a base that now carries this work. The PR title/description do not mention the column-stats changes; please reconcile scope and update the description, and confirm release-note labelling for this user-visible (experimental) behaviour.

The NaN/null-aware pruning logic itself (adjust_for_missing, only_nulls(), the isin/isnotin ALL_MATCH downgrade before the flip) is carefully reasoned and well covered by the new Python tests. One issue:

• ❌ Correctness (§9/§17): load_stats_by_column asserts data_type != UNKNOWN (column_stats_filter.cpp:201). A column that is wholly-null across every row-slice (e.g. sparsify_floats=True on an all-NaN column) produces a merged stats segment with only NAN_COUNT/NULL_COUNT columns and no MIN/MAX, so data_type stays UNKNOWN and this raises E_ASSERTION_FAILURE at filter time — even though values_for_column is explicitly written to handle the min-absent-but-counts-present case. Loading counts doesn't need data_type; skip the assertion / MIN/MAX visit_type block when data_type == UNKNOWN. An older client handles this data gracefully, so it's a regression for the all-null-column case. Inline comment posted; a test for a column null in all slices is missing.

This is a stacked PR; delta reviewed against 4dea3d1a.

[IvoDD]

I know this was the case before, but it seems odd to reraise everything as a NoDataFoundException.

I.e. surely we don't want to mask a network error as NoDataFound.
I'd be in favor of reraising the key_seg_pair_try.exception() type.

There are probably other places we do this so this should probably be a separate PR.

[alexowens90]

Agreed. Raising consistent exception types in general across all code paths for versions not existing/keys not existing would be a nice thing to do next time we're doing a major version bump for something more important. I found this crap as part of my refactor (which is the same for update), which is correctly a NoSuchVersionException for the batch methods.