-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
use specified comparator in CollapsedRangeDelMap #4386
use specified comparator in CollapsedRangeDelMap #4386
Conversation
The Comparator passed to CollapsedRangeDelMap was not used for operator less of the std::map `rep_` object contained in CollapsedRangeDelMap. So the map was always sorted using the default ByteWiseComparator, which seems wrong. Passing the specified Comparator through for usage in that map object fixes actual problems we were seeing with RangeDelete operations that do not delete keys as expected when using a custom Comparator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Do you mind providing details of the test crash in current master? For me they are passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@ajkr: re the failing test:
It seems that the deserialization of the truncated input string in the test works on my machine. If I make the input string one byte shorter as follows, then the test passes again: diff --git a/utilities/spatialdb/spatial_db_test.cc b/utilities/spatialdb/spatial_db_test.cc
index 783b347d0..be361bd62 100644
--- a/utilities/spatialdb/spatial_db_test.cc
+++ b/utilities/spatialdb/spatial_db_test.cc
@@ -94,7 +94,8 @@ TEST_F(SpatialDBTest, FeatureSetSerializeTest) {
ASSERT_EQ(deserialized.Get("m").get_double(), 3.25);
// corrupted serialization
- serialized = serialized.substr(0, serialized.size() - 4);
+ serialized = serialized.substr(0, serialized.size() - 5);
+
deserialized.Clear();
ASSERT_TRUE(!deserialized.Deserialize(serialized));
} Seems that for some reason the deserialization routine considers the 4-byte-truncated string valid on my machine, although I am wondering why this should be platform dependent. |
Summary: Add a unit test for range collapsing when non-default comparator is used. This exposes the bug fixed in facebook#4386. Pull Request resolved: facebook#4388 Differential Revision: D9918252 Pulled By: ajkr fbshipit-source-id: 99501b96b251eab41791a7e33b27055ee36c5c39
The Comparator passed to CollapsedRangeDelMap was not used for
operator less of the std::map
rep_
object contained inCollapsedRangeDelMap. So the map was always sorted using the
default ByteWiseComparator, which seems wrong.
Passing the specified Comparator through for usage in that map
object fixes actual problems we were seeing with RangeDelete operations
that do not delete keys as expected when using a custom Comparator.
I found that the tests in current master crash when I run them locally,
both with and without my patch, at the very same location. I therefore
don't know if the patch breaks something else, but it seems to fix
RangeDeletion issues in our product that uses RocksDB.