Skip to content

ARROW-14050: [C++] Make TDigest/Quantile kernels return nulls instead - #11199

Closed
lidavidm wants to merge 4 commits into
apache:masterfrom
lidavidm:arrow-14050
Closed

ARROW-14050: [C++] Make TDigest/Quantile kernels return nulls instead#11199
lidavidm wants to merge 4 commits into
apache:masterfrom
lidavidm:arrow-14050

Conversation

@lidavidm

Copy link
Copy Markdown
Member

As requested, this makes it so that TDigest/Quantile return nulls instead of empty arrays, making them easier to consume from bindings, and making them compose better with kernels like list_flatten and list_element.

@github-actions

Copy link
Copy Markdown

@ianmcook

Copy link
Copy Markdown
Member

Thanks for doing this @lidavidm! I'll test this with the R bindings later today.

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Just one question.

for (const auto& ty : {float64(), int64(), uint64()}) {
QuantileOptions options(std::vector<double>{0.0, 0.5, 1.0});
EXPECT_THAT(Quantile(*MakeScalar(ty, 1), options),
ResultWith(ArrayFromJSON(float64(), "[1.0, 1.0, 1.0]")));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you mean to also test with a null scalar here?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, added (also added to TDigest's tests)

@ianmcook

Copy link
Copy Markdown
Member

Works great from the R bindings. Thanks!

lidavidm added a commit that referenced this pull request Sep 24, 2021
This wraps the tdigest aggregation to make it easier for bindings to use. This PR is on top of ARROW-14050 (#11199).
Closes#11204 from lidavidm/arrow-14052
Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
As requested, this makes it so that TDigest/Quantile return nulls instead of empty arrays, making them easier to consume from bindings, and making them compose better with kernels like list_flatten and list_element.
Closesapache#11199 from lidavidm/arrow-14050
Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
This wraps the tdigest aggregation to make it easier for bindings to use. This PR is on top of ARROW-14050 (apache#11199).
Closesapache#11204 from lidavidm/arrow-14052
Authored-by: David Li <li.davidm96@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for freeto 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.

3 participants

@lidavidm@ianmcook@pitrou