-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[C++][Gandiva] Enhance random data generation #38569
Comments
Umm... in
When result-----------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
-----------------------------------------------------------------------------------------
TimedTestAdd3/min_time:1.000 4176 us 3081 us 437
TimedTestBigNested/min_time:1.000 6810 us 6538 us 214
TimedTestExtractYear/min_time:1.000 8837 us 8465 us 164
TimedTestFilterAdd2/min_time:1.000 5976 us 5955 us 234
TimedTestFilterLike/min_time:1.000 14700 us 14581 us 95
TimedTestCastFloatFromString/min_time:1.000 81841 us 81529 us 17
TimedTestCastIntFromString/min_time:1.000 46860 us 46665 us 30
+ TimedTestAllocs/min_time:1.000 128267 us 127612 us 11
+ TimedTestOutputStringAllocs/min_time:1.000 223284 us 222095 us 6
TimedTestMultiOr/min_time:1.000 10881 us 10841 us 125
TimedTestInExpr/min_time:1.000 24227 us 24171 us 57
DecimalAdd2Fast/min_time:1.000 3989 us 3968 us 353
DecimalAdd2LeadingZeroes/min_time:1.000 41976 us 41825 us 34
DecimalAdd2LeadingZeroesWithDiv/min_time:1.000 114398 us 113747 us 12
DecimalAdd2Large/min_time:1.000 118118 us 114793 us 12
DecimalAdd3Fast/min_time:1.000 4461 us 4411 us 322
DecimalAdd3LeadingZeroes/min_time:1.000 81679 us 81066 us 17
DecimalAdd3LeadingZeroesWithDiv/min_time:1.000 239198 us 238394 us 6
DecimalAdd3Large/min_time:1.000 240675 us 239581 us 6 |
Why is the range important in them? |
To conclude, it seems you are correct that the It seems correct that for the tests, the strings should be composed of lowercase letters only since the Thank you. Through the aspects you mentioned, I was able to understand the cause more clearly. It was possible to observe a performance difference for I have reviewed the following to understand why the range affects the benchmark: For clear verification, I performed the following actions on arrow/cpp/src/arrow/testing/random.cc Lines 366 to 407 in fc8c6b7
Benchmark results: Comparing it this way is helpful.
A - z
A - Z
a - z
A
a
B
a
Z-a
A-B
a-b
|
I am confused to deal with the The Using What would be the appropriate approach in terms of benchmark? |
Thanks for summarizing the behavior. String: Are you using https://github.com/apache/arrow/pull/38526/files#diff-b440faf74bbde4937a0a476511319f0c1cc255fbf0fb7372277c5f465df7a970R222-R229 ? If so, a diff --git a/cpp/src/gandiva/tests/micro_benchmarks.cc b/cpp/src/gandiva/tests/micro_benchmarks.cc
index 4bd4e8d51..88dd5a14c 100644
--- a/cpp/src/gandiva/tests/micro_benchmarks.cc
+++ b/cpp/src/gandiva/tests/micro_benchmarks.cc
@@ -318,7 +318,7 @@ static void TimedTestAllocs(benchmark::State& state) {
for (int i = 0; i < NUM_BATCHES; i++) {
for (int col = 0; col < num_fields; col++) {
arrays[col * NUM_BATCHES + i] =
- std::make_shared<ArrayPtr>(rag.String(num_batches, 0, 64, 0));
+ std::make_shared<ArrayPtr>(rag.String(num_batches, 64, 64, 0));
}
}
@@ -351,7 +351,7 @@ static void TimedTestOutputStringAllocs(benchmark::State& state) {
for (int i = 0; i < NUM_BATCHES; i++) {
for (int col = 0; col < num_fields; col++) {
arrays[col * NUM_BATCHES + i] =
- std::make_shared<ArrayPtr>(rag.String(num_batches, 0, 64, 0));
+ std::make_shared<ArrayPtr>(rag.String(num_batches, 64, 64, 0));
}
}
BTW, why did you compare What is the important point in Decimal: Could you explain more? diff --git a/cpp/src/gandiva/tests/micro_benchmarks.cc b/cpp/src/gandiva/tests/micro_benchmarks.cc
index 4bd4e8d51..02aed4c71 100644
--- a/cpp/src/gandiva/tests/micro_benchmarks.cc
+++ b/cpp/src/gandiva/tests/micro_benchmarks.cc
@@ -525,6 +526,7 @@ static void DoDecimalAdd2(benchmark::State& state, int32_t precision, int32_t sc
for (int col = 0; col < num_fields; col++) {
arrays[col * NUM_BATCHES + i] =
std::make_shared<ArrayPtr>(rag.Decimal128(decimal_type, num_batches, 0, 64, 0));
+ std::cout << **arrays[col * NUM_BATCHES + i] << std::endl;
}
}
It seems that all of these values have different precision. (The |
I think in both cases precision is fixed. In |
Thank you for your response. For string
You are right. We are working on Ganvida's benchmark. In summary, it appears that:
previous generated string data
To briefly explain why we conducted the test, I've observed that the previous benchmarks selected characters within the # previous
- TimedTestAllocs/min_time:1.000 140487 us 137767 us 10
- TimedTestOutputStringAllocs/min_time:1.000 228228 us 226211 us 6
# random.h
+ TimedTestAllocs/min_time:1.000 243130 us 242760 us 6
+ TimedTestOutputStringAllocs/min_time:1.000 332357 us 331799 us 4 Upon further inspection, particularly of the My understanding of Gandiva is limited, but I believe that mixing For decimalIn this case, I think you should check the And for the existing decimal generation code, it was implemented like this arrow/cpp/src/gandiva/tests/generate_data.h Lines 82 to 98 in e62ec62
Anyway, I'm having a hard time figuring out how to fit this in because for previous generated DecimalAdd2LeadingZeroes data
`random.h` generated DecimalAdd2LeadingZeroes data
previous generated DecimalAdd2LeadingZeroesWithDiv data
`random.h` generated DecimalAdd2LeadingZeroes data
|
Oh I see your point now. Their precisions are indeed different. |
Describe the enhancement requested
Refactor random generation utilizing random.h instead of generate_data.h.
This addresses the issue.
Improvement
Remaining tasks
The following issues still need to be resolved.
DecimalAdd2Large
andDecimalAdd3Large
Question
TimedTestAllocs, TimedTestOutputStringAllocsas-is
to-be
Component(s)
C++ - Gandiva
The text was updated successfully, but these errors were encountered: