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/12/14 23:58:47 UTC

[GitHub] [arrow] augustoasilva opened a new pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

augustoasilva opened a new pull request #11957:
URL: https://github.com/apache/arrow/pull/11957


   Revert change on Gandiva's cache policy


-- 
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 #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#issuecomment-994150821






-- 
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] augustoasilva commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772257623



##########
File path: cpp/src/gandiva/projector.cc
##########
@@ -18,14 +18,15 @@
 #include "gandiva/projector.h"
 
 #include <memory>
+#include <thread>

Review comment:
       I did not saw it, so removed.




-- 
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 #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#issuecomment-998492975


   Benchmark runs are scheduled for baseline = 9ac9405503a7013da256811dba65994f0ff045f1 and contender = 02e9464c506ed46883b17fd9b1576ff547b09c18. 02e9464c506ed46883b17fd9b1576ff547b09c18 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/b10e5088ab72423a9ee86a2dfe30337a...fb76270cca444a65a84c75dddb58eb3d/)
   [Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/589134153ae84833b97b5a44509eb882...2a8580aa3a234e3aac0c8f6b4f153f0e/)
   [Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2b593c15093f4c558cc22e98139a5cf5...99b4a136a4bf4ad084f608bd185cd6e2/)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


-- 
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] pravindra closed pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
pravindra closed pull request #11957:
URL: https://github.com/apache/arrow/pull/11957


   


-- 
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] projjal commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772278154



##########
File path: cpp/src/gandiva/gandiva_object_cache.cc
##########
@@ -17,34 +17,25 @@
 
 #include "gandiva/gandiva_object_cache.h"
 
+#include <utility>
+
 namespace gandiva {
 
 GandivaObjectCache::GandivaObjectCache(
     std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>&
         cache,
     ExpressionCacheKey key)
-    : cache_key_(key) {
+    : cache_key_(std::move(key)) {

Review comment:
       its ok here.




-- 
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] projjal commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772270173



##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -18,13 +18,16 @@
 #include "gandiva/filter.h"
 
 #include <memory>
+#include <thread>

Review comment:
       These includes are also not necessary. Can you review all the includes in the PR.




-- 
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] augustoasilva commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772271146



##########
File path: cpp/src/gandiva/gandiva_object_cache.cc
##########
@@ -17,34 +17,25 @@
 
 #include "gandiva/gandiva_object_cache.h"
 
+#include <utility>
+
 namespace gandiva {
 
 GandivaObjectCache::GandivaObjectCache(
     std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>&
         cache,
     ExpressionCacheKey key)
-    : cache_key_(key) {
+    : cache_key_(std::move(key)) {

Review comment:
       This change us a good pratice for creating an object which has parameter as a smart pointer. I've read about it on some online discussions. Should I revert this change?




-- 
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 #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#issuecomment-998492975


   Benchmark runs are scheduled for baseline = 9ac9405503a7013da256811dba65994f0ff045f1 and contender = 02e9464c506ed46883b17fd9b1576ff547b09c18. 02e9464c506ed46883b17fd9b1576ff547b09c18 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/b10e5088ab72423a9ee86a2dfe30337a...fb76270cca444a65a84c75dddb58eb3d/)
   [Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/589134153ae84833b97b5a44509eb882...2a8580aa3a234e3aac0c8f6b4f153f0e/)
   [Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2b593c15093f4c558cc22e98139a5cf5...99b4a136a4bf4ad084f608bd185cd6e2/)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


-- 
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] augustoasilva commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
augustoasilva commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772277647



##########
File path: cpp/src/gandiva/filter.cc
##########
@@ -18,13 +18,16 @@
 #include "gandiva/filter.h"
 
 #include <memory>
+#include <thread>

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

[GitHub] [arrow] ursabot edited a comment on pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#issuecomment-998492975


   Benchmark runs are scheduled for baseline = 9ac9405503a7013da256811dba65994f0ff045f1 and contender = 02e9464c506ed46883b17fd9b1576ff547b09c18. 02e9464c506ed46883b17fd9b1576ff547b09c18 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/b10e5088ab72423a9ee86a2dfe30337a...fb76270cca444a65a84c75dddb58eb3d/)
   [Finished :arrow_down:0.45% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/589134153ae84833b97b5a44509eb882...2a8580aa3a234e3aac0c8f6b4f153f0e/)
   [Finished :arrow_down:0.62% :arrow_up:0.18%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2b593c15093f4c558cc22e98139a5cf5...99b4a136a4bf4ad084f608bd185cd6e2/)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


-- 
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 #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#issuecomment-998492975


   Benchmark runs are scheduled for baseline = 9ac9405503a7013da256811dba65994f0ff045f1 and contender = 02e9464c506ed46883b17fd9b1576ff547b09c18. 02e9464c506ed46883b17fd9b1576ff547b09c18 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/b10e5088ab72423a9ee86a2dfe30337a...fb76270cca444a65a84c75dddb58eb3d/)
   [Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/589134153ae84833b97b5a44509eb882...2a8580aa3a234e3aac0c8f6b4f153f0e/)
   [Finished :arrow_down:0.62% :arrow_up:0.18%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2b593c15093f4c558cc22e98139a5cf5...99b4a136a4bf4ad084f608bd185cd6e2/)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


-- 
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] projjal commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772269291



##########
File path: cpp/src/gandiva/gandiva_object_cache.cc
##########
@@ -17,34 +17,25 @@
 
 #include "gandiva/gandiva_object_cache.h"
 
+#include <utility>
+
 namespace gandiva {
 
 GandivaObjectCache::GandivaObjectCache(
     std::shared_ptr<Cache<ExpressionCacheKey, std::shared_ptr<llvm::MemoryBuffer>>>&
         cache,
     ExpressionCacheKey key)
-    : cache_key_(key) {
+    : cache_key_(std::move(key)) {

Review comment:
       why this change?




-- 
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] projjal commented on a change in pull request #11957: ARROW-15110: [C++][Gandiva] Revert change on Gandiva's cache policy

Posted by GitBox <gi...@apache.org>.
projjal commented on a change in pull request #11957:
URL: https://github.com/apache/arrow/pull/11957#discussion_r772084750



##########
File path: cpp/src/gandiva/projector.cc
##########
@@ -18,14 +18,15 @@
 #include "gandiva/projector.h"
 
 #include <memory>
+#include <thread>

Review comment:
       why are these being included?




-- 
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