You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/10/17 00:46:33 UTC
[GitHub] [arrow] westonpace opened a new pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
westonpace opened a new pull request #11441:
URL: https://github.com/apache/arrow/pull/11441
This estimates the combined size (in bytes) of all buffers referenced by various containers. This will be needed for tracing and profiling the query engine. For example, we may want to report how much data is passing through a given exec node over 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
[GitHub] [arrow] westonpace commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735991929
##########
File path: cpp/src/arrow/util/byte_size_test.cc
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <gtest/gtest.h>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+#include "arrow/testing/generator.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+namespace util {
+
+TEST(TotalBufferSize, Arrays) {
Review comment:
Good idea. I added one.
--
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
[GitHub] [arrow] westonpace commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958863916
CI failures are unrelated. I'm going to 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
[GitHub] [arrow] westonpace commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958863916
CI failures are unrelated. I'm going to 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
[GitHub] [arrow] ursabot commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] ursabot commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] westonpace closed pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace closed pull request #11441:
URL: https://github.com/apache/arrow/pull/11441
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Finished :arrow_down:1.03% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Finished :arrow_down:0.62% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] lidavidm commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-951316499
Ok, that sounds good to me. I'll give this another look now.
--
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
[GitHub] [arrow] lidavidm commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-951895383
Well, it doesn't replicate locally. I'll kick the build again.
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Finished :arrow_down:0.62% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735977290
##########
File path: cpp/src/arrow/util/byte_size_test.cc
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <gtest/gtest.h>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+#include "arrow/testing/generator.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+namespace util {
+
+TEST(TotalBufferSize, Arrays) {
+ std::shared_ptr<Array> no_nulls = ArrayFromJSON(int16(), "[1, 2, 3]");
+ ASSERT_EQ(6, TotalBufferSize(*no_nulls->data()));
+
+ std::shared_ptr<Array> with_nulls =
+ ArrayFromJSON(int16(), "[1, 2, 3, 4, null, 6, 7, 8, 9]");
+ ASSERT_EQ(20, TotalBufferSize(*with_nulls->data()));
+}
+
+TEST(TotalBufferSize, ArrayWithOffset) {
+ std::shared_ptr<Array> base_array =
+ ArrayFromJSON(int16(), "[1, 2, 3, 4, null, 6, 7, 8, 9]");
+ std::shared_ptr<Array> sliced = base_array->Slice(8, 1);
+ ASSERT_EQ(20, TotalBufferSize(*sliced));
+}
+
+TEST(TotalBufferSize, ArrayWithDict) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(dictionary(int32(), int8()), "[0, 0, 0]");
Review comment:
Ah thanks, I didn't realize it would do the encoding for you.
--
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
[GitHub] [arrow] ursabot commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] westonpace commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958863916
CI failures are unrelated. I'm going to 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
[GitHub] [arrow] westonpace commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-951315539
@lidavidm My work on the more sophisticated method is coming along but rather than replace this I think it will be an addition to this. So there will be two ways of calculating the byte size. With that in mind I renamed the function to `TotalBufferSize` and added detection of duplicate buffers. This basically gives you the amount of RAM being kept alive by this datum (although there may be other owners of that RAM, so there is no guarantee removing the datum will free up that much RAM). I also simplified the test cases a bit by using ArrayFromJSON.
--
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
[GitHub] [arrow] lidavidm commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-951980112
Ah, I would guess that the `enable_if_floating_point` template is getting shadowed in a unity build, and that my local build happened to combine different source files so it doesn't replicate…and the only reason why it pops up here is because this makes enough changes that CMake combines different source files than before.
I filed ARROW-14475 but I think for now it can be fixed by adjusting aggregate_basic_internal.h to not use enable_if_floating_point.
--
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
[GitHub] [arrow] lidavidm commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-952023105
Or #11547 should hopefully fix it.
--
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
[GitHub] [arrow] westonpace commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-953292248
Thanks for figuring this out. I've rebased and will merge on green.
--
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
[GitHub] [arrow] westonpace commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735972119
##########
File path: cpp/src/arrow/datum.cc
##########
@@ -128,6 +129,25 @@ int64_t Datum::length() const {
}
}
+int64_t Datum::TotalBufferSize() const {
+ switch (this->kind()) {
+ case Datum::ARRAY:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<ArrayData>>(this->value));
+ case Datum::CHUNKED_ARRAY:
+ return util::TotalBufferSize(
+ *util::get<std::shared_ptr<ChunkedArray>>(this->value));
+ case Datum::RECORD_BATCH:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<RecordBatch>>(this->value));
+ case Datum::TABLE:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<Table>>(this->value));
+ case Datum::SCALAR:
+ return 0;
Review comment:
Yeah, I'd rather defer until someone has a need for it. At the moment the wrapper metadata (e.g. ExecBatch, etc.) is likely to be larger than the scalar itself.
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
--
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
[GitHub] [arrow] ursabot commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] westonpace closed pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace closed pull request #11441:
URL: https://github.com/apache/arrow/pull/11441
--
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
[GitHub] [arrow] ursabot edited a comment on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958865836
Benchmark runs are scheduled for baseline = 7ffeead4000b0419946ea4f7785fad5157e1fb9d and contender = 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619. 7190a94c6eeb2c0ad4e7b8662534e7ed2d277619 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/8a2c1be9011b49af92d414600d494975...332c552306ab43ef9365508678a16030/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/c0280687800248f7891e392947ad897b...297b7a64f2d54639af15afcfa960af13/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/126b07822e0144099064c1b0249ecf7e...d87badb1968741be86cbcc96b5d6f649/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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
[GitHub] [arrow] westonpace commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-958863916
CI failures are unrelated. I'm going to 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
[GitHub] [arrow] westonpace closed pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace closed pull request #11441:
URL: https://github.com/apache/arrow/pull/11441
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735956880
##########
File path: cpp/src/arrow/util/byte_size_test.cc
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <gtest/gtest.h>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+#include "arrow/testing/generator.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+namespace util {
+
+TEST(TotalBufferSize, Arrays) {
+ std::shared_ptr<Array> no_nulls = ArrayFromJSON(int16(), "[1, 2, 3]");
+ ASSERT_EQ(6, TotalBufferSize(*no_nulls->data()));
+
+ std::shared_ptr<Array> with_nulls =
+ ArrayFromJSON(int16(), "[1, 2, 3, 4, null, 6, 7, 8, 9]");
+ ASSERT_EQ(20, TotalBufferSize(*with_nulls->data()));
+}
+
+TEST(TotalBufferSize, ArrayWithOffset) {
+ std::shared_ptr<Array> base_array =
+ ArrayFromJSON(int16(), "[1, 2, 3, 4, null, 6, 7, 8, 9]");
+ std::shared_ptr<Array> sliced = base_array->Slice(8, 1);
+ ASSERT_EQ(20, TotalBufferSize(*sliced));
+}
+
+TEST(TotalBufferSize, ArrayWithDict) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(dictionary(int32(), int8()), "[0, 0, 0]");
Review comment:
Is this a valid dictionary array? (Where's the dictionary?) There's a separate helper called DictArrayFromJSON for this kind of use so you can define both the dictionary and the indices.
##########
File path: cpp/src/arrow/util/byte_size_test.cc
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <gtest/gtest.h>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+#include "arrow/testing/generator.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+namespace util {
+
+TEST(TotalBufferSize, Arrays) {
Review comment:
Can we also add a test of a nested type (struct, list, etc)?
##########
File path: cpp/src/arrow/util/byte_size.cc
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <cstdint>
+#include <unordered_set>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+
+namespace arrow {
+
+namespace util {
+
+namespace {
+
+int64_t DoTotalBufferSize(const ArrayData& array_data,
+ std::unordered_set<const uint8_t*>* seen_buffers) {
+ int64_t sum = 0;
+ for (const auto& buffer : array_data.buffers) {
+ if (buffer && seen_buffers->insert(buffer->data()).second) {
Review comment:
(Though, probably that won't come up often?)
##########
File path: cpp/src/arrow/datum.cc
##########
@@ -128,6 +129,25 @@ int64_t Datum::length() const {
}
}
+int64_t Datum::TotalBufferSize() const {
+ switch (this->kind()) {
+ case Datum::ARRAY:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<ArrayData>>(this->value));
+ case Datum::CHUNKED_ARRAY:
+ return util::TotalBufferSize(
+ *util::get<std::shared_ptr<ChunkedArray>>(this->value));
+ case Datum::RECORD_BATCH:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<RecordBatch>>(this->value));
+ case Datum::TABLE:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<Table>>(this->value));
+ case Datum::SCALAR:
+ return 0;
Review comment:
I suppose it is well-documented in the docstring.
##########
File path: cpp/src/arrow/util/byte_size.cc
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <cstdint>
+#include <unordered_set>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+
+namespace arrow {
+
+namespace util {
+
+namespace {
+
+int64_t DoTotalBufferSize(const ArrayData& array_data,
+ std::unordered_set<const uint8_t*>* seen_buffers) {
+ int64_t sum = 0;
+ for (const auto& buffer : array_data.buffers) {
+ if (buffer && seen_buffers->insert(buffer->data()).second) {
Review comment:
This won't catch the case where one buffer is a subset of another, right?
##########
File path: cpp/src/arrow/datum.cc
##########
@@ -128,6 +129,25 @@ int64_t Datum::length() const {
}
}
+int64_t Datum::TotalBufferSize() const {
+ switch (this->kind()) {
+ case Datum::ARRAY:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<ArrayData>>(this->value));
+ case Datum::CHUNKED_ARRAY:
+ return util::TotalBufferSize(
+ *util::get<std::shared_ptr<ChunkedArray>>(this->value));
+ case Datum::RECORD_BATCH:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<RecordBatch>>(this->value));
+ case Datum::TABLE:
+ return util::TotalBufferSize(*util::get<std::shared_ptr<Table>>(this->value));
+ case Datum::SCALAR:
+ return 0;
Review comment:
Is it worth filing a follow-up for this? I imagine most scalars are fairly trivial but strings may not necessarily be.
--
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
[GitHub] [arrow] westonpace commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735980483
##########
File path: cpp/src/arrow/util/byte_size.cc
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <cstdint>
+#include <unordered_set>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+
+namespace arrow {
+
+namespace util {
+
+namespace {
+
+int64_t DoTotalBufferSize(const ArrayData& array_data,
+ std::unordered_set<const uint8_t*>* seen_buffers) {
+ int64_t sum = 0;
+ for (const auto& buffer : array_data.buffers) {
+ if (buffer && seen_buffers->insert(buffer->data()).second) {
Review comment:
That's a fair point. I'd been so caught up in zero-copy array slicing I didn't realize there was zero-copy buffer slicing. There is a PR up for removing duplicate ranges. It looks like I will need to do that here as well. I will add a note at least to the comments for now and then probably solve duplicate ranges the same across both methods.
--
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
[GitHub] [arrow] lidavidm commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r730847404
##########
File path: cpp/src/arrow/util/byte_size.cc
##########
@@ -0,0 +1,67 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+namespace arrow {
+
+namespace util {
+
+int64_t EstimateBufferSize(const ArrayData& array_data) {
+ int64_t sum = 0;
+ for (const auto& buffer : array_data.buffers) {
+ if (buffer) {
+ sum += buffer->size();
+ }
+ }
+ for (const auto& child : array_data.child_data) {
+ sum += EstimateBufferSize(*child);
+ }
+ return sum;
Review comment:
What about the dictionary (if present)?
--
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
[GitHub] [arrow] westonpace commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735976797
##########
File path: cpp/src/arrow/util/byte_size_test.cc
##########
@@ -0,0 +1,94 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <gtest/gtest.h>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+#include "arrow/testing/generator.h"
+#include "arrow/testing/gtest_util.h"
+
+namespace arrow {
+namespace util {
+
+TEST(TotalBufferSize, Arrays) {
+ std::shared_ptr<Array> no_nulls = ArrayFromJSON(int16(), "[1, 2, 3]");
+ ASSERT_EQ(6, TotalBufferSize(*no_nulls->data()));
+
+ std::shared_ptr<Array> with_nulls =
+ ArrayFromJSON(int16(), "[1, 2, 3, 4, null, 6, 7, 8, 9]");
+ ASSERT_EQ(20, TotalBufferSize(*with_nulls->data()));
+}
+
+TEST(TotalBufferSize, ArrayWithOffset) {
+ std::shared_ptr<Array> base_array =
+ ArrayFromJSON(int16(), "[1, 2, 3, 4, null, 6, 7, 8, 9]");
+ std::shared_ptr<Array> sliced = base_array->Slice(8, 1);
+ ASSERT_EQ(20, TotalBufferSize(*sliced));
+}
+
+TEST(TotalBufferSize, ArrayWithDict) {
+ std::shared_ptr<Array> arr = ArrayFromJSON(dictionary(int32(), int8()), "[0, 0, 0]");
Review comment:
It's kind of a pointless dictionary array (since the index type is larger than the value type) but I think it is valid. `ArrayFromJSON` should be able to create dictionaries under the hood. You should get the same thing as running dictionary encode on the JSON parsed to the value type. I think you only need `DictArrayFromJSON` if you want to specify the indices yourself. The length is what you would expect for 3 32-bit index values (12 bytes) and a single 8-bit dictionary (1 byte). There is no validity map because there are no nulls.
--
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
[GitHub] [arrow] westonpace commented on a change in pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#discussion_r735986749
##########
File path: cpp/src/arrow/util/byte_size.cc
##########
@@ -0,0 +1,107 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/util/byte_size.h"
+
+#include <cstdint>
+#include <unordered_set>
+
+#include "arrow/array.h"
+#include "arrow/buffer.h"
+
+namespace arrow {
+
+namespace util {
+
+namespace {
+
+int64_t DoTotalBufferSize(const ArrayData& array_data,
+ std::unordered_set<const uint8_t*>* seen_buffers) {
+ int64_t sum = 0;
+ for (const auto& buffer : array_data.buffers) {
+ if (buffer && seen_buffers->insert(buffer->data()).second) {
Review comment:
ARROW-14357
--
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
[GitHub] [arrow] lidavidm commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-951886682
Oooh, the AppVeyor failure looks real, but entirely unrelated to what's going on here. I'll dig into it.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11441:
URL: https://github.com/apache/arrow/pull/11441#issuecomment-944837166
https://issues.apache.org/jira/browse/ARROW-14355
--
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
[GitHub] [arrow] westonpace closed pull request #11441: ARROW-14355: [C++] Create naive implementation of algorithm to estimate table/batch buffer size
Posted by GitBox <gi...@apache.org>.
westonpace closed pull request #11441:
URL: https://github.com/apache/arrow/pull/11441
--
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