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

Add a new dummy compression called NULL #7798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dbeck
Copy link
Contributor

@dbeck dbeck commented Mar 6, 2025

This fixes bug #7714 where adding a column with a default value (jargon: missing value) and a compressed batch with all nulls created an ambiguity. In the all null cases the compressed block was stored as a NULL value.

With this change, I introduce a new special compression type, the 'NULL' compression which is a single byte place holder for an 'all-null' compressed block. This allows us to distinguish between the missing value vs the all-null values.

Please note that the wrong results impacted existing tests so I updated the expected results, as well as I added reference queries before compression to prove the updated values were wrong before.

A new debug only GUC was added for testing a future upgrade script, which will arrive as a separate PR.

Fixes #7714

Copy link

codecov bot commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 77.77778% with 16 lines in your changes missing coverage. Please review.

Project coverage is 81.90%. Comparing base (59f50f2) to head (977c85b).
Report is 806 commits behind head on main.

Files with missing lines Patch % Lines
tsl/test/src/compression_unit_test.c 65.38% 0 Missing and 9 partials ⚠️
tsl/src/compression/algorithms/null.c 68.75% 0 Missing and 5 partials ⚠️
tsl/src/compression/compression.c 95.00% 0 Missing and 1 partial ⚠️
tsl/src/hypercore/hypercore_handler.c 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7798      +/-   ##
==========================================
+ Coverage   80.06%   81.90%   +1.83%     
==========================================
  Files         190      248      +58     
  Lines       37181    45531    +8350     
  Branches     9450    11393    +1943     
==========================================
+ Hits        29770    37293    +7523     
- Misses       2997     3749     +752     
- Partials     4414     4489      +75     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dbeck dbeck force-pushed the bug-7714-004-squashed-wrong-result-default-value branch from 5e48616 to ebd2b32 Compare March 6, 2025 15:54
@dbeck dbeck requested a review from fabriziomello March 6, 2025 15:54
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.

Let's merge this w/o the ts_is_attribute_missing, it's a separate function and I think we might want to improve it. The fix itself is fine.

@dbeck dbeck force-pushed the bug-7714-004-squashed-wrong-result-default-value branch 4 times, most recently from 0a129d7 to 899adf1 Compare March 6, 2025 17:40
@dbeck dbeck enabled auto-merge (rebase) March 6, 2025 17:48
@mkindahl mkindahl changed the title Add a new dummy comression called NULL Add a new dummy compression called NULL Mar 7, 2025
@dbeck dbeck force-pushed the bug-7714-004-squashed-wrong-result-default-value branch from 899adf1 to 7a2bc7e Compare March 7, 2025 12:58
@philkra philkra added this to the v2.19.0 milestone Mar 7, 2025
@dbeck dbeck force-pushed the bug-7714-004-squashed-wrong-result-default-value branch 2 times, most recently from faaa1da to d162f9c Compare March 7, 2025 20:20
This fixes bug timescale#7714 where adding a column with a default
value (jargon: missing value) and a compressed batch with
all nulls created an ambiguity. In the all null cases the
compressed block was stored as a NULL value.

With this change, I introduce a new special compression
type, the 'NULL' compression which is a single byte place
holder for an 'all-null' compressed block. This allows us
to distinguish between the missing value vs the all-null
values.

Please note that the wrong results impacted existing tests
so I updated the expected results, as well as I added
reference queries before compression to prove the updated
values were wrong before.

A new debug only GUC was added for testing a future upgrade
script, which will arrive as a separate PR.

Fixes timescale#7714
@dbeck dbeck force-pushed the bug-7714-004-squashed-wrong-result-default-value branch from d162f9c to 977c85b Compare March 8, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Wrong result returned after compressing column, with alter table, new column with default value
4 participants