Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modularize batch queue in transparent decompression #6363

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

erimatnor
Copy link
Contributor

@erimatnor erimatnor commented Nov 30, 2023

The batch queue implementation, which is part of transparent decompression, is refactored to be its own self-contained module. Previously, the batch queue functionality consisted of functions that operated on the DecompressChunkState node instead of its own distinct object.

The two batch queue types, heap and fifo, are now modularized in their separate source files with a common interface exposed in batch_queue.h. The state related to the two batch queue implementations is moved from DecompressChunkState into the respective modules depending on type. For example, the heap state and related initialization code is moved to the heap-based batch queue.

The DecompressContext is also moved to its own header file so that other modules need not include everything related to DecompressChunkState.

In a separate commit, the DecompressChunkColumnDescription is renamed to CompressionColumnDescription to reflect that this type is decoupled from DecompressChunkState.

Disable-check: force-changelog-file
Disable-check: commit-count

@erimatnor erimatnor self-assigned this Nov 30, 2023
@@ -0,0 +1,79 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Contents moved from exec.h

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (9276312) 86.92% compared to head (6d40139) 59.44%.

Files Patch % Lines
tsl/src/nodes/decompress_chunk/compressed_batch.c 78.84% 0 Missing and 11 partials ⚠️
tsl/src/nodes/decompress_chunk/batch_queue_heap.c 90.66% 0 Missing and 7 partials ⚠️
tsl/src/nodes/decompress_chunk/exec.c 89.28% 0 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6363       +/-   ##
===========================================
- Coverage   86.92%   59.44%   -27.49%     
===========================================
  Files         250      251        +1     
  Lines       58155    50844     -7311     
  Branches    12970    12847      -123     
===========================================
- Hits        50550    30222    -20328     
- Misses       5205    16134    +10929     
- Partials     2400     4488     +2088     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erimatnor erimatnor marked this pull request as ready for review November 30, 2023 11:14
@erimatnor erimatnor force-pushed the refactor-batch-queue branch 3 times, most recently from 2520c25 to 36881f6 Compare November 30, 2023 16:19
Copy link
Member

@akuzm akuzm left a comment

Choose a reason for hiding this comment

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

Looks good now, let's also run tsbench for it to double-check.

Copy link
Contributor

@mkindahl mkindahl left a comment

Choose a reason for hiding this comment

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

Mostly nits and some things I wonder if we need, but nothing that should block merging.

tsl/src/nodes/decompress_chunk/batch_queue_fifo.c Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue_fifo.h Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue.h Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue.h Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue_heap.c Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue_heap.c Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue_heap.c Outdated Show resolved Hide resolved
tsl/src/nodes/decompress_chunk/batch_queue_heap.c Outdated Show resolved Hide resolved
@erimatnor erimatnor force-pushed the refactor-batch-queue branch 2 times, most recently from fd3565e to 7b852d6 Compare December 7, 2023 08:11
The batch queue implementation, which is part of transparent
decompression, is refactored to be its own self-contained
module. Previously, the batch queue functionality consisted of
functions that operated on the DecompressChunkState node instead of
its own distinct object.

The two batch queue types, heap and fifo, are now modularized in their
separate source files with a common interface exposed in
`batch_queue.h`. The state related to the two batch queue
implementations is moved from DecompressChunkState into the respective
modules depending on type. For example, the heap state and related
initialization code is moved to the heap-based batch queue.

The DecompressContext is also moved to its own header file so that
other modules need not include everything related to
DecompressChunkState.
Rename DecompressChunkColumnDescription to
CompressionColumnDescription to reflect that this type is decoupled
from DecompressChunkState.
@erimatnor erimatnor merged commit aacc6b5 into timescale:main Dec 8, 2023
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants