Skip to content

ASV benchmarks for bool column truncation#3054

Merged
IvoDD merged 1 commit intomasterfrom
arrow-truncated-bool-benchmark
Apr 29, 2026
Merged

ASV benchmarks for bool column truncation#3054
IvoDD merged 1 commit intomasterfrom
arrow-truncated-bool-benchmark

Conversation

@IvoDD
Copy link
Copy Markdown
Collaborator

@IvoDD IvoDD commented Apr 22, 2026

Reference Issues/PRs

New benchmarks for memcpy bool truncation.
Useful for benchmarking follow up PR #3052

What does this implement or fix?

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@IvoDD IvoDD added the patch Small change, should increase patch version label Apr 22, 2026
@IvoDD IvoDD added the no-release-notes This PR shouldn't be added to release notes. label Apr 22, 2026

def time_read_row_range_byte_unaligned(self, rows, sparsity):
# Should read the first two blocks and truncate both to a byte unaligned offsets
self.lib.read(self.sym, row_range=(5, 100_005))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider adding peakmem_read_row_range_byte_aligned and peakmem_read_row_range_byte_unaligned counterparts to match the existing time_*/peakmem_* pairing pattern used for time_read/peakmem_read and time_write/peakmem_write. Memory usage during truncation may differ meaningfully between aligned and unaligned cases.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

ArcticDB Code Review Summary

API & Compatibility

  • No breaking changes to public Python API
  • On-disk format unchanged
  • No protobuf or key type changes

Memory & Safety

  • No C++ changes; not applicable

Correctness

  • Row range values are correct: (8, 100_008) produces byte-aligned offsets (8 is divisible by 8) within each 100K-row block; (5, 100_005) produces byte-unaligned offsets (5 is not divisible by 8)
  • Comments accurately explain the intent of the specific values

Code Quality

  • Missing peakmem_read_row_range_byte_aligned / peakmem_read_row_range_byte_unaligned counterparts -- the class uses time_*/peakmem_* pairs throughout; memory profiling during truncation may differ between aligned and unaligned cases (inline comment posted)

Testing

  • This PR is the test/benchmark addition; no behavioural code changed

Build & Dependencies

  • No CMake or dependency changes

Security

  • No credentials or boundary violations

PR Title & Description

  • Title is clear and concise
  • Description wording is confusing: Useful for benchmarking follow up PR 3052 -- since PR 3052 has a lower number it presumably predates this PR; clarify whether these benchmarks measure the already-merged 3052 work or whether 3052 is forthcoming and depends on these benchmarks landing first
  • PR checklist items left unchecked (likely fine for a benchmarks-only PR, but worth acknowledging)

Documentation

  • No public API changes; no documentation updates needed

Side-effect note (informational): The setup_cache_key values for ArrowStrings benchmarks changed from arrow:263 to arrow:271 in benchmarks.json because inserting 8 new lines into ArrowBools shifted those line numbers. This is correct ASV behaviour and will simply invalidate the cached setup for those benchmarks on the next run -- no action needed.

@IvoDD IvoDD merged commit b7017b9 into master Apr 29, 2026
268 of 274 checks passed
@IvoDD IvoDD deleted the arrow-truncated-bool-benchmark branch April 29, 2026 11:57
maxim-morozov pushed a commit that referenced this pull request Apr 29, 2026
#### Reference Issues/PRs
New benchmarks for memcpy bool truncation.
Useful for benchmarking follow up PR #3052 

#### What does this implement or fix?

#### Any other comments?

#### Checklist

<details>
  <summary>
   Checklist for code changes...
  </summary>
 
- [ ] Have you updated the relevant docstrings, documentation and
copyright notice?
- [ ] Is this contribution tested against [all ArcticDB's
features](../docs/mkdocs/docs/technical/contributing.md)?
- [ ] Do all exceptions introduced raise appropriate [error
messages](https://docs.arcticdb.io/error_messages/)?
 - [ ] Are API changes highlighted in the PR description?
- [ ] Is the PR labelled as enhancement or bug so it appears in
autogenerated release notes?
</details>

<!--
Thanks for contributing a Pull Request to ArcticDB! Please ensure you
have taken a look at:
- ArcticDB's Code of Conduct:
https://github.com/man-group/ArcticDB/blob/master/CODE_OF_CONDUCT.md
- ArcticDB's Contribution Licensing:
https://github.com/man-group/ArcticDB/blob/master/docs/mkdocs/docs/technical/contributing.md#contribution-licensing
-->

Co-authored-by: Ivo <ivo.dilov@man.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release-notes This PR shouldn't be added to release notes. patch Small change, should increase patch version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants