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 treat_null_as_missing option to locf #1067

Merged
merged 2 commits into from Feb 15, 2019

Conversation

Projects
None yet
3 participants
@svenklemm
Copy link
Member

commented Feb 15, 2019

When doing a gapfill query with multiple columns that may contain
NULLs it is not trivial to remove NULL values from individual columns
with a WHERE clause, this new locf option allows those NULL values
to be ignored in gapfill queries with locf.

@svenklemm svenklemm requested a review from cevian Feb 15, 2019

@svenklemm svenklemm changed the title Add ignore_nulls option locf Add ignore_nulls option to locf Feb 15, 2019

@svenklemm svenklemm added this to the 1.3.0 milestone Feb 15, 2019

@svenklemm svenklemm force-pushed the svenklemm:locf_ignore_nulls branch from 9adeb6a to 017803f Feb 15, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 15, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@6ee217f). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1067   +/-   ##
=========================================
  Coverage          ?   91.82%           
=========================================
  Files             ?       91           
  Lines             ?    10796           
  Branches          ?        0           
=========================================
  Hits              ?     9913           
  Misses            ?      883           
  Partials          ?        0
Impacted Files Coverage Δ
tsl/src/gapfill/locf.c 100% <100%> (ø)
tsl/src/gapfill/exec.c 96.1% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee217f...017803f. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Feb 15, 2019

Codecov Report

Merging #1067 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
- Coverage   91.87%   91.82%   -0.05%     
==========================================
  Files          91       91              
  Lines       10788    10796       +8     
==========================================
+ Hits         9911     9913       +2     
- Misses        877      883       +6
Impacted Files Coverage Δ
tsl/src/gapfill/locf.c 100% <100%> (ø) ⬆️
tsl/src/gapfill/exec.c 96.1% <100%> (+0.02%) ⬆️
src/loader/bgw_message_queue.c 88.11% <0%> (-4.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee217f...0f478a9. Read the comment docs.

@davidkohn88
Copy link
Contributor

left a comment

This looks good to me, the only thing I'd say is that we probably don't want the argument to be called ignore_nulls, because the booleans seem flipped the wrong way for me on that, if I set ignore_nulls = TRUE I no longer ignore nulls and instead fill them in. I think something like fill_nulls would be better.

@svenklemm svenklemm force-pushed the svenklemm:locf_ignore_nulls branch from 017803f to 6e02714 Feb 15, 2019

@svenklemm svenklemm changed the title Add ignore_nulls option to locf Add treat_null_as_missing option to locf Feb 15, 2019

@cevian

cevian approved these changes Feb 15, 2019

ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("invalid locf argument: treat_null_as_missing must be a BOOL literal")));
if (!treat_null_as_missing->constisnull)

This comment has been minimized.

Copy link
@cevian

cevian Feb 15, 2019

Contributor

I wonder if we should error on constisnull too? That seems like a weird usage.

This comment has been minimized.

Copy link
@svenklemm

svenklemm Feb 15, 2019

Author Member

if its NULL it will use default value so i think this is fine

@svenklemm svenklemm force-pushed the svenklemm:locf_ignore_nulls branch 5 times, most recently from 13c786e to 4cadd07 Feb 15, 2019

Add treat_null_as_missing option to locf
When doing a gapfill query with multiple columns that may contain
NULLs it is not trivial to remove NULL values from individual columns
with a WHERE clause, this new locf option allows those NULL values
to be ignored in gapfill queries with locf.

We drop the old locf function because we dont want 2 locf functions.
Unfortunately this means any views using locf have to be dropped.

@svenklemm svenklemm force-pushed the svenklemm:locf_ignore_nulls branch 2 times, most recently from f5f3eb8 to 53a1f9c Feb 15, 2019

@svenklemm svenklemm force-pushed the svenklemm:locf_ignore_nulls branch from c979c52 to 0f478a9 Feb 15, 2019

@svenklemm svenklemm merged commit cdcd8c9 into timescale:master Feb 15, 2019

5 checks passed

codecov/patch 100% of diff hit (target 91.87%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +8.12% compared to 6ee217f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@cevian cevian added the gapfill label May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.