You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2025/01/14 17:55:18 UTC
[PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
pitrou opened a new pull request, #45259:
URL: https://github.com/apache/arrow/pull/45259
### Rationale for this change
Add a rank_percentile function following the Wikipedia definition:
https://en.wikipedia.org/wiki/Percentile_rank
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes, an additional compute function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2590723984
@zanmato1984 @jorisvandenbossche Do you want to take a look at this?
Naming-wise, I hesitated between "rank_percentile" and "percentile_rank". I chose the former for better tab-completion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2598408459
https://github.com/pitrou/arrow/pull/13 filed. @pitrou PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2602027997
Ok, this is fixed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2597891126
Hi @pitrou , I want to suggest a refinement that simplifies the code structure of your current pr. I think the best way to show it is via actual code changes, so may I open up a pr targeting your branch? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2591871242
`rank_quantile` sounds ok to me. I find occurrences of "quantile rank" on Google, so it's not unheard of.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2591496930
If the `factor` is free to specify, is the name `rank_quantile` more accurate? (BTW I prefer `rank_xxx` to `xxx_rank`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2606483156
Yes, I was hoping for opinions on the general idea and API, but perhaps we can simply merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1926267973
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
I don't have enough experience of using functions alike so I'll respect others' opinions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #45259:
URL: https://github.com/apache/arrow/pull/45259
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1922597461
##########
cpp/src/arrow/compute/kernels/vector_rank.cc:
##########
@@ -15,9 +15,13 @@
// specific language governing permissions and limitations
// under the License.
+#include <functional>
+#include <memory>
+
#include "arrow/compute/function.h"
#include "arrow/compute/kernels/vector_sort_internal.h"
#include "arrow/compute/registry.h"
+#include "arrow/util/logging.h"
Review Comment:
No, I forgot to remove this. Will do :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1926007389
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
I don't see it being too useful - I think most of the cases (0, 1) is enough. 0..100 is mostly used for display purpose and not actual computation in my experience.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1925527236
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
Since we do have time maybe its worth removing the factor to simplify? What is the advantage of including into the QuantileRanker class versus return something in the range 0..1 and having the user supply the factor if they need?
I figure the advantage to taking the factor out would be to simplify the implementation and give more room for vectorized multiplication applications
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2601994870
Seems the latest force push doesn't include the changes I've made. Anything wrong? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1923084202
##########
cpp/src/arrow/compute/kernels/vector_sort_test.cc:
##########
@@ -2466,5 +2469,151 @@ TEST_F(TestRank, EmptyChunks) {
}
}
+class TestRankQuantile : public BaseTestRank {
+ public:
+ void AssertRankQuantile(const DatumVector& datums, SortOrder order,
+ NullPlacement null_placement, double factor,
+ const std::shared_ptr<Array>& expected) {
+ const std::vector<SortKey> sort_keys{SortKey("foo", order)};
+ RankQuantileOptions options(sort_keys, null_placement, factor);
+ ARROW_SCOPED_TRACE("options = ", options.ToString());
+ for (const auto& datum : datums) {
+ ASSERT_OK_AND_ASSIGN(auto actual, CallFunction("rank_quantile", {datum}, &options));
+ ValidateOutput(actual);
+ AssertDatumsEqual(expected, actual, /*verbose=*/true);
+ }
+ }
+
+ void AssertRankQuantile(const DatumVector& datums, SortOrder order,
+ NullPlacement null_placement, double factor,
+ const std::string& expected) {
+ AssertRankQuantile(datums, order, null_placement, factor,
+ ArrayFromJSON(float64(), expected));
+ }
+
+ void AssertRankQuantile(SortOrder order, NullPlacement null_placement, double factor,
+ const std::shared_ptr<Array>& expected) {
+ AssertRankQuantile(datums_, order, null_placement, factor, expected);
+ }
+
+ void AssertRankQuantile(SortOrder order, NullPlacement null_placement, double factor,
+ const std::string& expected) {
+ AssertRankQuantile(datums_, order, null_placement, factor,
+ ArrayFromJSON(float64(), expected));
+ }
+
+ void AssertRankQuantileEmpty(std::shared_ptr<DataType> type) {
+ for (auto null_placement : AllNullPlacements()) {
+ for (auto order : AllOrders()) {
+ AssertRankQuantile({ArrayFromJSON(type, "[]")}, order, null_placement,
+ /*factor=*/1.0, "[]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null]")}, order, null_placement,
+ /*factor=*/1.0, "[0.5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null]")}, order, null_placement,
+ /*factor=*/10.0, "[5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null, null, null]")}, order,
+ null_placement, /*factor=*/1.0, "[0.5, 0.5, 0.5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null, null, null]")}, order,
+ null_placement, /*factor=*/100.0, "[50, 50, 50]");
+ }
+ }
+ }
+
+ // Expecting an input ordered like [1, 2, 1, 2, 1]
+ void AssertRankQuantile_12121() {
+ for (auto null_placement : AllNullPlacements()) {
+ AssertRankQuantile(SortOrder::Ascending, null_placement, 100.0,
+ "[30.0, 80.0, 30.0, 80.0, 30.0]");
+ AssertRankQuantile(SortOrder::Descending, null_placement, 100.0,
+ "[70.0, 20.0, 70.0, 20.0, 70.0]");
+ }
+ }
+
+ // Expecting an input ordered like [null, 1, null, 2, null]
+ void AssertRankQuantile_N1N2N() {
+ AssertRankQuantile(SortOrder::Ascending, NullPlacement::AtStart, 1.0,
+ "[0.3, 0.7, 0.3, 0.9, 0.3]");
+ AssertRankQuantile(SortOrder::Ascending, NullPlacement::AtEnd, 1.0,
+ "[0.7, 0.1, 0.7, 0.3, 0.7]");
+ AssertRankQuantile(SortOrder::Descending, NullPlacement::AtStart, 1.0,
+ "[0.3, 0.9, 0.3, 0.7, 0.3]");
+ AssertRankQuantile(SortOrder::Descending, NullPlacement::AtEnd, 1.0,
+ "[0.7, 0.3, 0.7, 0.1, 0.7]");
+ }
+
+ void AssertRankQuantileNumeric(std::shared_ptr<DataType> type) {
+ ARROW_SCOPED_TRACE("type = ", type->ToString());
+ AssertRankQuantileEmpty(type);
+
+ // Reproduce the example from https://en.wikipedia.org/wiki/Quantile_rank
Review Comment:
```suggestion
// Reproduce the example from https://en.wikipedia.org/wiki/Percentile_rank
```
##########
cpp/src/arrow/compute/kernels/vector_sort_test.cc:
##########
@@ -2466,5 +2469,151 @@ TEST_F(TestRank, EmptyChunks) {
}
}
+class TestRankQuantile : public BaseTestRank {
+ public:
+ void AssertRankQuantile(const DatumVector& datums, SortOrder order,
+ NullPlacement null_placement, double factor,
+ const std::shared_ptr<Array>& expected) {
+ const std::vector<SortKey> sort_keys{SortKey("foo", order)};
+ RankQuantileOptions options(sort_keys, null_placement, factor);
+ ARROW_SCOPED_TRACE("options = ", options.ToString());
+ for (const auto& datum : datums) {
+ ASSERT_OK_AND_ASSIGN(auto actual, CallFunction("rank_quantile", {datum}, &options));
+ ValidateOutput(actual);
+ AssertDatumsEqual(expected, actual, /*verbose=*/true);
+ }
+ }
+
+ void AssertRankQuantile(const DatumVector& datums, SortOrder order,
+ NullPlacement null_placement, double factor,
+ const std::string& expected) {
+ AssertRankQuantile(datums, order, null_placement, factor,
+ ArrayFromJSON(float64(), expected));
+ }
+
+ void AssertRankQuantile(SortOrder order, NullPlacement null_placement, double factor,
+ const std::shared_ptr<Array>& expected) {
+ AssertRankQuantile(datums_, order, null_placement, factor, expected);
+ }
+
+ void AssertRankQuantile(SortOrder order, NullPlacement null_placement, double factor,
+ const std::string& expected) {
+ AssertRankQuantile(datums_, order, null_placement, factor,
+ ArrayFromJSON(float64(), expected));
+ }
+
+ void AssertRankQuantileEmpty(std::shared_ptr<DataType> type) {
+ for (auto null_placement : AllNullPlacements()) {
+ for (auto order : AllOrders()) {
+ AssertRankQuantile({ArrayFromJSON(type, "[]")}, order, null_placement,
+ /*factor=*/1.0, "[]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null]")}, order, null_placement,
+ /*factor=*/1.0, "[0.5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null]")}, order, null_placement,
+ /*factor=*/10.0, "[5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null, null, null]")}, order,
+ null_placement, /*factor=*/1.0, "[0.5, 0.5, 0.5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null, null, null]")}, order,
+ null_placement, /*factor=*/100.0, "[50, 50, 50]");
+ }
+ }
+ }
+
+ // Expecting an input ordered like [1, 2, 1, 2, 1]
+ void AssertRankQuantile_12121() {
+ for (auto null_placement : AllNullPlacements()) {
+ AssertRankQuantile(SortOrder::Ascending, null_placement, 100.0,
+ "[30.0, 80.0, 30.0, 80.0, 30.0]");
+ AssertRankQuantile(SortOrder::Descending, null_placement, 100.0,
Review Comment:
You might want `/*factor=*/100.0` for this as well as the following cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_percentile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2597977897
Yes, you can!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "icexelloss (via GitHub)" <gi...@apache.org>.
icexelloss commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1926007389
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
I don't see it being too useful - I think most of the cases (0, 1) is enough
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1927364814
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
Ok, I removed the `factor` option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2606505056
You can keep this open as long as you feel necessary for more ideas or comments. I was just saying I have no more to comment :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2606495973
Sorry I did not comment sooner but I think the idea and API are great. If anything, it could be simpler to remove the factor and just leave it to the end user to specify that, but I don't think it's a big deal either way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #45259:
URL: https://github.com/apache/arrow/pull/45259#issuecomment-2606367288
This is good to go at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1923343948
##########
cpp/src/arrow/compute/kernels/vector_sort_test.cc:
##########
@@ -2466,5 +2469,151 @@ TEST_F(TestRank, EmptyChunks) {
}
}
+class TestRankQuantile : public BaseTestRank {
+ public:
+ void AssertRankQuantile(const DatumVector& datums, SortOrder order,
+ NullPlacement null_placement, double factor,
+ const std::shared_ptr<Array>& expected) {
+ const std::vector<SortKey> sort_keys{SortKey("foo", order)};
+ RankQuantileOptions options(sort_keys, null_placement, factor);
+ ARROW_SCOPED_TRACE("options = ", options.ToString());
+ for (const auto& datum : datums) {
+ ASSERT_OK_AND_ASSIGN(auto actual, CallFunction("rank_quantile", {datum}, &options));
+ ValidateOutput(actual);
+ AssertDatumsEqual(expected, actual, /*verbose=*/true);
+ }
+ }
+
+ void AssertRankQuantile(const DatumVector& datums, SortOrder order,
+ NullPlacement null_placement, double factor,
+ const std::string& expected) {
+ AssertRankQuantile(datums, order, null_placement, factor,
+ ArrayFromJSON(float64(), expected));
+ }
+
+ void AssertRankQuantile(SortOrder order, NullPlacement null_placement, double factor,
+ const std::shared_ptr<Array>& expected) {
+ AssertRankQuantile(datums_, order, null_placement, factor, expected);
+ }
+
+ void AssertRankQuantile(SortOrder order, NullPlacement null_placement, double factor,
+ const std::string& expected) {
+ AssertRankQuantile(datums_, order, null_placement, factor,
+ ArrayFromJSON(float64(), expected));
+ }
+
+ void AssertRankQuantileEmpty(std::shared_ptr<DataType> type) {
+ for (auto null_placement : AllNullPlacements()) {
+ for (auto order : AllOrders()) {
+ AssertRankQuantile({ArrayFromJSON(type, "[]")}, order, null_placement,
+ /*factor=*/1.0, "[]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null]")}, order, null_placement,
+ /*factor=*/1.0, "[0.5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null]")}, order, null_placement,
+ /*factor=*/10.0, "[5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null, null, null]")}, order,
+ null_placement, /*factor=*/1.0, "[0.5, 0.5, 0.5]");
+ AssertRankQuantile({ArrayFromJSON(type, "[null, null, null]")}, order,
+ null_placement, /*factor=*/100.0, "[50, 50, 50]");
+ }
+ }
+ }
+
+ // Expecting an input ordered like [1, 2, 1, 2, 1]
+ void AssertRankQuantile_12121() {
+ for (auto null_placement : AllNullPlacements()) {
+ AssertRankQuantile(SortOrder::Ascending, null_placement, 100.0,
+ "[30.0, 80.0, 30.0, 80.0, 30.0]");
+ AssertRankQuantile(SortOrder::Descending, null_placement, 100.0,
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1922593947
##########
cpp/src/arrow/compute/kernels/vector_rank.cc:
##########
@@ -15,9 +15,13 @@
// specific language governing permissions and limitations
// under the License.
+#include <functional>
+#include <memory>
+
#include "arrow/compute/function.h"
#include "arrow/compute/kernels/vector_sort_internal.h"
#include "arrow/compute/registry.h"
+#include "arrow/util/logging.h"
Review Comment:
Ever used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1925704140
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
Yes, we can probably simplify this. Multiplication optimizations would be nice as well, but certainly not critical given than sorting and computing the rankings is bound to be much slower than the final multiplication step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45190: [C++][Compute] Add rank_quantile function [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #45259:
URL: https://github.com/apache/arrow/pull/45259#discussion_r1925704684
##########
cpp/src/arrow/compute/api_vector.cc:
##########
@@ -151,6 +152,10 @@ static auto kRankOptionsType = GetFunctionOptionsType<RankOptions>(
DataMember("sort_keys", &RankOptions::sort_keys),
DataMember("null_placement", &RankOptions::null_placement),
DataMember("tiebreaker", &RankOptions::tiebreaker));
+static auto kRankQuantileOptionsType = GetFunctionOptionsType<RankQuantileOptions>(
+ DataMember("sort_keys", &RankQuantileOptions::sort_keys),
+ DataMember("null_placement", &RankQuantileOptions::null_placement),
+ DataMember("factor", &RankQuantileOptions::factor));
Review Comment:
@zanmato1984 @cyb70289 @icexelloss Do you think the factor would be useful to keep?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org