ARROW-4167: [C++][Gandiva] Switch to arrow/util/variant

[pravindra]

No description provided.

[codecov-io]

Codecov Report

Merging #3425 into master will increase coverage by 0.85%.
The diff coverage is 40.9%.

@@            Coverage Diff             @@
##           master    #3425      +/-   ##
==========================================
+ Coverage   88.32%   89.18%   +0.85%     
==========================================
  Files         631      407     -224     
  Lines       79308    50432   -28876     
  Branches     1069        0    -1069     
==========================================
- Hits        70048    44977   -25071     
+ Misses       9145     5455    -3690     
+ Partials      115        0     -115

Impacted Files	Coverage Δ
cpp/src/gandiva/to_date_holder.cc	58.33% <0%> (ø)	⬆️
cpp/src/gandiva/like_holder.cc	90.24% <100%> (ø)	⬆️
cpp/src/gandiva/node.h	100% <100%> (ø)	⬆️
cpp/src/gandiva/llvm_generator.cc	83.08% <35.29%> (ø)	⬆️
cpp/src/arrow/csv/reader.h	0% <0%> (-100%)	⬇️
cpp/src/arrow/csv/reader.cc	0.52% <0%> (-90.48%)	⬇️
cpp/src/arrow/adapters/orc/adapter.cc	0.26% <0%> (-74.08%)	⬇️
cpp/src/plasma/eviction_policy.cc	59.01% <0%> (-40.99%)	⬇️
cpp/src/plasma/events.cc	52.5% <0%> (-35%)	⬇️
cpp/src/arrow/csv/options.h	66.66% <0%> (-33.34%)	⬇️
... and 277 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3cd36a...94bb9f5. Read the comment docs.

[pravindra]

@wesm @pitrou can you please review?

[wesm]

FYI it is only necessary to explicitly ask for code review if more than ~24 hours have elapsed. You can safely assume that we are seeing every pull request

[wesm]

+1. Do any of the following usages of Boost leak in your public API?

cpp/src/gandiva/lru_cache.h:#include <boost/optional.hpp>
cpp/src/gandiva/lru_cache.h:// modified from boost LRU cache -> the boost cache supported only an
cpp/src/gandiva/lru_cache.h:  boost::optional<value_type> get(const key_type& key) {
cpp/src/gandiva/lru_cache.h:      return boost::none;
cpp/src/gandiva/lru_cache_test.cc:  ASSERT_EQ(cache_.get(TestCacheKey(1)), boost::none);
cpp/src/gandiva/function_signature.cc:#include "boost/functional/hash.hpp"
cpp/src/gandiva/function_signature.cc:  boost::hash_combine(result, base_name_);
cpp/src/gandiva/function_signature.cc:  boost::hash_combine(result, ret_type_->id());
cpp/src/gandiva/function_signature.cc:    boost::hash_combine(result, param_type->id());
cpp/src/gandiva/expression_registry.cc:#include "boost/iterator/transform_iterator.hpp"
cpp/src/gandiva/expr_validator.h:#include "boost/functional/hash.hpp"
cpp/src/gandiva/expr_validator.h:  using FieldMap = std::unordered_map<std::string, FieldPtr, boost::hash<std::string>>;
cpp/src/gandiva/configuration.cc:#include "boost/functional/hash.hpp"
cpp/src/gandiva/configuration.cc:  boost::hash<std::string> string_hash;
cpp/src/gandiva/filter_cache_key.h:#include "boost/functional/hash.hpp"
cpp/src/gandiva/filter_cache_key.h:    boost::hash_combine(result, expression_as_string_);
cpp/src/gandiva/filter_cache_key.h:    boost::hash_combine(result, configuration);
cpp/src/gandiva/filter_cache_key.h:    boost::hash_combine(result, schema_->ToString());
cpp/src/gandiva/filter_cache_key.h:    boost::hash_combine(result, uniqifier_);
cpp/src/gandiva/date_utils.h:  static date_format_converter sql_date_format_to_boost_map_;
cpp/src/gandiva/date_utils.cc:  for (const auto& it : sql_date_format_to_boost_map_) {
cpp/src/gandiva/date_utils.cc:        builder << sql_date_format_to_boost_map_[potentialList[0]];
cpp/src/gandiva/date_utils.cc:              builder << sql_date_format_to_boost_map_[match];
cpp/src/gandiva/date_utils.cc:      builder << sql_date_format_to_boost_map_[exactMatches[0]];
cpp/src/gandiva/date_utils.cc:DateUtils::date_format_converter DateUtils::sql_date_format_to_boost_map_ = InitMap();
cpp/src/gandiva/projector_cache_key.h:      boost::hash_combine(result, expr_as_string);
cpp/src/gandiva/projector_cache_key.h:    boost::hash_combine(result, configuration);
cpp/src/gandiva/projector_cache_key.h:    boost::hash_combine(result, schema_->ToString());
cpp/src/gandiva/projector_cache_key.h:    boost::hash_combine(result, uniqifier_);
cpp/src/gandiva/cache.h:    boost::optional<ValueType> result;
cpp/src/gandiva/cache.h:    return result != boost::none ? *result : nullptr;

[pravindra]

Do any of the following usages of Boost leak in your public API?

@wesm, I'm assuming the includes in .cc files can't leak. The rest are in cache/validation, which aren't leaked to the public API.

we are now installing all header files, so this can break easily. any best practices to ensure that the header files having the public APIs don't include the internal header files ?

[kou]

How about using "internal" for internal header file names and using these internal header files only from source files?
ARROW_INSTALL_ALL_HEADERS removes header files that include "internal" in their file names: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/BuildUtils.cmake#L609

[pravindra]

How about using "internal" for internal header file names and using these internal header files only from source files?
ARROW_INSTALL_ALL_HEADERS removes header files that include "internal" in their file names: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/BuildUtils.cmake#L609

lgtm - but it causes a mismatch between the class-name and file-name. eg. class LRUCache will be declared in lru_cache_internal.h and defined in lru_cache.cc.