Skip to content
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.

Sign up for GitHub

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

Jump to bottom

maint: Remove use of folly/hash/Hash.h #1506

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

maint: Remove use of folly/hash/Hash.h #1506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions cpp/arcticdb/entity/atom_key.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <arcticdb/entity/index_range.hpp>
#include <variant>
#include <optional>
#include <boost/container_hash/hash.hpp>
#include <fmt/format.h>

namespace arcticdb::entity {
Expand Down Expand Up @@ -101,10 +102,14 @@ class AtomKeyImpl {

size_t get_cached_hash() const {
if (!hash_) {
// arcticdb::commutative_hash_combine needs extra template specialisations for our variant types, folly's
// built-in variant forwards to std::hash which should be good enough for these simple types
hash_ = folly::hash::hash_combine(id_, version_id_, creation_ts_, content_hash_, key_type_, index_start_,
index_end_);
auto seed = hash(id_);
boost::hash_combine(seed, version_id_);
boost::hash_combine(seed, creation_ts_);
boost::hash_combine(seed, content_hash_);
boost::hash_combine(seed, key_type_);
boost::hash_combine(seed, index_start_);
boost::hash_combine(seed, index_end_);
hash_ = seed;
Comment on lines +105 to +112
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the returned values of get_cached_hash must not change for backward compatibility (e.g. to be able to retrieve segments which have been stored in databases in previous versions).

Is there a way we could test that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's the case, however I agree we should check. I believe we have version backwards and forwards compatibility checks, I'll find out where they are and point you at them

}
return *hash_;
}
Expand Down
2 changes: 0 additions & 2 deletions cpp/arcticdb/util/hash.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
#include <cstdint>
#include <string_view>

#include <folly/hash/Hash.h>

#define XXH_STATIC_LINKING_ONLY
#include <xxhash.h>
#undef XXH_STATIC_LINKING_ONLY
Expand Down
67 changes: 0 additions & 67 deletions cpp/arcticdb/util/test/test_hash.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,70 +29,3 @@ TEST(HashAccum, NotCommutative) {
EXPECT_NE(h1.digest(), h2.digest());

}

TEST(HashComm, Commutative) {
using namespace arcticdb;
using namespace folly::hash;

auto h = commutative_hash_combine(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);

EXPECT_EQ(h, commutative_hash_combine(10, 9, 8, 7, 6, 5, 4, 3, 2, 1));
EXPECT_EQ(h, commutative_hash_combine(10, 2, 3, 4, 5, 6, 7, 8, 9, 1));

EXPECT_EQ(
commutative_hash_combine(12345, 6789),
commutative_hash_combine(6789, 12345)
);

auto v1 = commutative_hash_combine(0, 1629821389809000000,std::string("FIXT.1.1"),454.0,std::string("IOI"),3224.0,std::string("MA"),std::string("20210824-16:09:49.809"),std::string("GLGMKTLIST2"),std::string("IOIID-1629821389809"),std::string("REPLACE"),std::string("[N/A]"),std::string("437076CG5"),1.0,std::string("US437076CG52"),4,std::string("CORP"),std::string("USD_HGD"),20210826,std::string("BUY"),800000.0,std::string("USD"),std::string("SPREAD"),std::string("20210824-16:07:49"),std::string("20210824-16:05:49"),std::string("912810SX7"),1.0,std::string("MKTX"),std::string("C"),std::string("CONTRA_FIRM"),std::string("Spread"),120.0,std::string("MarketList-Orders"),std::string("Did Not Trade"),std::string("Holding_Bin"),std::string("OneStep"),std::string("4 2"),std::string("YES"),0.0,std::string("N"),std::string("N"),std::string("ILQD"),10467391.0,2.0);
auto v2 = commutative_hash_combine(0, 1629821389809000000,std::string("FIXT.1.1"),454.0,std::string("IOI"),3224.0,std::string("MA"),std::string("20210824-16:09:49.809"),std::string("GLGMKTLIST2"),std::string("IOIID-1629821389809"),std::string("REPLACE"),10467391.0,std::string("[N/A]"),std::string("437076CG5"),1.0,std::string("US437076CG52"),4,std::string("CORP"),std::string("USD_HGD"),20210826,std::string("BUY"),800000.0,std::string("USD"),std::string("SPREAD"),std::string("20210824-16:07:49"),std::string("20210824-16:05:49"),std::string("912810SX7"),1.0,std::string("MKTX"),std::string("C"),std::string("CONTRA_FIRM"),std::string("Spread"),120.0,std::string("MarketList-Orders"),std::string("Did Not Trade"),std::string("Holding_Bin"),std::string("OneStep"),std::string("4 2"),std::string("YES"),2.0,0.0,std::string("N"),std::string("N"),std::string("ILQD"));

std::cout<<v1<<std::endl;
std::cout<<v2<<std::endl;

EXPECT_EQ(v1, v2);

float f1 = 1234.5;
float f2 = 78.65;

float g1 = 1234.5;
float g2 = 78.65;

EXPECT_EQ(
commutative_hash_combine(f1, f2),
commutative_hash_combine(g2, g1)
);

std::cout<<commutative_hash_combine(9726480045861996436ULL, std::string("10442909"))<<std::endl;
std::cout<<commutative_hash_combine(17243764687685379754ULL, std::string("[N/A]"))<<std::endl;
std::cout<<commutative_hash_combine(9457389251931416297ULL, std::string("98956PAF9"))<<std::endl;
std::cout<<commutative_hash_combine(2648263869243483102ULL, std::string("1"))<<std::endl;
std::cout<<commutative_hash_combine(10692736793407104629ULL, std::string("US98956PAF99"))<<std::endl;
std::cout<<commutative_hash_combine(7633205480517386763ULL, std::string("4"))<<std::endl;
std::cout<<commutative_hash_combine(8481507868260942362ULL, std::string("CORP"))<<std::endl;
std::cout<<commutative_hash_combine(4419353893253087336ULL, std::string("USD_HGD"))<<std::endl;
std::cout<<commutative_hash_combine(10212419605264796417ULL, std::string("20210824"))<<std::endl;

std::cout<<commutative_hash_combine(9726480045861996436ULL, std::string("10442909"), std::string("[N/A]")) <<std::endl;

auto h1 = commutative_hash_combine_generic(9726480045861996436ULL, folly::Hash{}, std::string("10442909"), std::string("[N/A]"));
auto h2 = commutative_hash_combine_generic(9726480045861996436ULL, folly::Hash{}, std::string("10442909"));
auto h3 = commutative_hash_combine_generic(h2, folly::Hash{}, std::string("[N/A]"));

auto h4 = commutative_hash_combine_generic(9726480045861996436ULL, folly::Hash{}, std::string("[N/A]"));
auto h5 = commutative_hash_combine_generic(h4, folly::Hash{}, std::string("10442909"));

EXPECT_EQ(h1, h3);
EXPECT_EQ(h1, h5);

auto j1 = commutative_hash_combine(9726480045861996436ULL, std::string("10442909"), std::string("[N/A]"));
auto j2 = commutative_hash_combine(9726480045861996436ULL, std::string("10442909"));
auto j3 = commutative_hash_combine(j2, std::string("[N/A]"));

auto j4 = commutative_hash_combine(9726480045861996436ULL, std::string("[N/A]"));
auto j5 = commutative_hash_combine(j4, std::string("10442909"));

EXPECT_NE(j1, j3);
EXPECT_NE(j1, j5);
EXPECT_NE(j3, j5);
}
Loading