Port FSTLevelDBKey to C++ #937

wilhuff · 2018-03-16T20:04:41Z

This is just the main class and tests but none of the usages. I'll migrate those separately.

These operations universally operate on keys obtained from leveldb so it's actually unhelpful to force all the callers to make absl::string_views from them.

var-const

LGTM with minor nits.

var-const · 2018-03-18T20:02:01Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+   * Creates a key prefix that points just before the first key for the given
+   * user_id.
+   */
+  static std::string KeyPrefix(const absl::string_view user_id);


Nit: remove const from declarations (I think applies to all usage of absl::string_view as a parameter in this file). (I would guess these were const references at some point)

Done. They weren't const references before, but this was a style that Zhi was applying that I carried over.

var-const · 2018-03-18T20:03:41Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+   * Decodes the given complete key, storing the decoded values as properties of
+   * the receiver.
+   *
+   * @return true if the key successfully decoded, NO otherwise. If NO is


Does "NO" come from an Objective-C comment?

Sorry: BOOL, YES, and NO are all the Objective-C spellings of bool, true, and false. These files were copied from Objective-C and then massaged until they worked.

Fixed.

var-const · 2018-03-18T20:04:53Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+   *
+   * @return true if the key successfully decoded, NO otherwise. If NO is
+   * returned, the properties of the receiver are in an undefined state until
+   * the next call to -decode_key:.


"-decode_key:" looks a little weird. There are several similar "-foo:" names in comments in this file.

var-const · 2018-03-18T20:06:22Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+  static std::string KeyPrefix();
+
+  /**
+   * Creates a complete key that points to a specific document. The document_key


Nit: there is mismatch between "document_key" here and the name of the argument which is just key.

Fixed. This should have been document_key. This was the only instance of this I could find.

var-const · 2018-03-18T20:10:11Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+   * Key()).
+   *
+   * @return true if the key successfully decoded, false otherwise. If false is
+   * returned, the properties of the receiver are in an undefined state until


Nit: is "receiver" a carryover from Objective-C?

Yes. Fixed.

var-const · 2018-03-18T21:30:55Z

Firestore/core/src/firebase/firestore/model/types.h

+ * BatchId is a locally assigned ID for a batch of mutations that have been
+ * applied.
+ */
+typedef int32_t BatchId;


Nit: consider using using instead of typedef (style guide recommends using for new code).

var-const · 2018-03-18T21:32:13Z

Firestore/core/src/firebase/firestore/util/string_util.cc

@@ -48,6 +48,11 @@ std::string ImmediateSuccessor(absl::string_view s) {
  return out;
 }

+bool StartsWith(const std::string &value, const std::string &prefix) {
+  return prefix.size() <= value.size() &&


You could use absl::StartsWith.

var-const · 2018-03-19T17:23:06Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+ * natively has concepts of tables and indexes. Where there's overlap, a comment
+ * denotes the server value from the storage_format_internal.proto.
+ */
+enum ComponentLabel {


Optional: ToTW 86 recommends enum class for new code. If you feel it's more verbose for little gain, I'm fine with plain enum as well.

Yeah. I explicitly chose an old-school enum specifically because it's weakly typed and implicitly converts to an integer. I don't see much value in forcing static_cast in every caller.

var-const · 2018-03-19T18:26:15Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+bool LevelDbMutationKey::Decode(leveldb::Slice key) {
+  user_id_.clear();
+
+  return ReadTableNameMatching(&key, kMutationsTable) &&


I wish some of this boilerplate could be reduced, but don't really have a good suggestion. The best I can come up with is having a helper method that represents each of the Key classes as a tuple, and then Encode/Decode methods could delegate to a variadic template function like void LevelDbMutationKey::Encode() { EncodeImpl(AsTuple(*this)); } However, it would require too much reworking, and will also increase binary size somewhat.

Even with a template mechanism each of these function invocations becomes an instantiation of the placeholder in the tuple. At best I think it ends up removing the "Read", the "&key, " and changing the "&&" into "," which isn't really enough savings to justify this kind of reengineering.

The gain would be removing the duplication between Decode and Key (the common source for errors in serializing/deserializing is when those get out of sync) - both would delegate the tuple representation to a common helper. But yeah, it still won't outweigh the downsides.

I see what you mean. That's a significant chunk of work and I'm still not entirely sure how I'd make partial keys work there.

var-const · 2018-03-19T18:38:18Z

Firestore/core/test/firebase/firestore/local/leveldb_key_test.cc

+  // it's vital that partial segments in common not be prefixes.
+  ASSERT_TRUE(util::StartsWith(foo_user_key, table_key));
+
+  // Here even though "" is a prefix of "foo" that prefix is within a segment so


Nit: commas before "that" and before "so".

var-const

LGTM.

var-const · 2018-03-21T19:51:43Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+  static std::string KeyPrefix(const absl::string_view user_id);
+
+  /** Creates a complete key that points to a specific user_id and batch_id. */
+  static std::string Key(const absl::string_view user_id,


Hmm, a little, but feel free to leave as is. My take is that these FooKey classes are domain objects that are serialized to/from strings. But it's a small enough nitpick to just leave as is.

var-const · 2018-03-21T19:52:43Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+   * returned, the properties of the receiver are in an undefined state until
+   * the next call to -decode_key:.
+   */
+  bool Decode(leveldb::Slice key);


It's just that in general, so far it seems that our code base leaves towards immutable classes, so an outlier stands out. Thanks for expanding the comments.

var-const · 2018-03-21T19:57:18Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+
+ private:
+  std::string user_id_;
+  model::BatchId batch_id_;


I think an uninitialized variable always has a potential of becoming an issue in the future in case code is refactored. There would be indeed a double initialization, but is this really so performance-critical that the cost of initializing an int is a concern (string is probably more expensive to initialize anyway)? OTOH, since this is by design in current code, I'm okay if you leave it as is (perhaps add a short comment like "Deliberately uninitialized - will be deserialized").

var-const · 2018-03-21T19:57:48Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+ * A key in the mutation_queues table.
+ *
+ * Note that where mutation_queues contains one row about each queue, mutations
+ * contains the actual mutation batches themselves.


Ah, I missed that, thanks.

var-const · 2018-03-21T19:58:28Z

Firestore/core/src/firebase/firestore/local/leveldb_key.h

+  static std::string KeyPrefix(const model::ResourcePath& resource_path);
+
+  /**
+   * Decodes the contents of a remote document key into properties on this


Sorry about that. I like the new version.

var-const · 2018-03-21T20:00:07Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+ * If the read is successful, returns true and contents will be updated to the
+ * next unread byte.
+ */
+bool ReadComponentLabelMatching(leveldb::Slice *contents,


Since these are local to this file, I'm fine with leaving it be.

var-const · 2018-03-21T20:08:31Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+  leveldb::Slice complete_segments = *contents;
+
+  std::string segment;
+  std::vector<std::string> path_segments{};


It's just redundant in this case, but feel free to leave them in, it's no big deal.

Default- vs value-initialization matters for primitives and for classes with no default constructor containing primitives:

struct Pod1 { int x; }; Pod1 foo; assert(foo.x == 0); // Undefined; x is uninitialized Pod1 bar{}; assert(bar.x == 0); // True struct Pod2 { int x = 42; }; Pod2 spam; assert(spam.x == 42); // True Pod2 eggs{}; assert(eggs.x == 42); // True

But both vector and string are well-behaved classes.

var-const · 2018-03-21T20:15:32Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+bool LevelDbMutationKey::Decode(leveldb::Slice key) {
+  user_id_.clear();
+
+  return ReadTableNameMatching(&key, kMutationsTable) &&


The gain would be removing the duplication between Decode and Key (the common source for errors in serializing/deserializing is when those get out of sync) - both would delegate the tuple representation to a common helper. But yeah, it still won't outweigh the downsides.

wilhuff added the api: firestore label Mar 16, 2018

wilhuff assigned var-const Mar 16, 2018

wilhuff requested a review from var-const March 16, 2018 20:04

googlebot added the cla: yes label Mar 16, 2018

wilhuff force-pushed the wilhuff/leveldb-key branch from 39c4019 to 43c41d1 Compare March 16, 2018 22:35

wilhuff added 6 commits March 16, 2018 16:49

Initial plumbing for leveldb local parts

866aa5d

Add leveldb_key_test.cc to the project

8d71c74

Add model::BatchId

1402bff

Add string StartsWith

caeffff

Port leveldb_key.{h,cc}

ee12694

Revert back to using leveldb::Slice for read/describe

16a9bc7

These operations universally operate on keys obtained from leveldb so it's actually unhelpful to force all the callers to make absl::string_views from them.

wilhuff force-pushed the wilhuff/leveldb-key branch from 43c41d1 to 16a9bc7 Compare March 18, 2018 00:27

var-const suggested changes Mar 19, 2018

View reviewed changes

Fixup leveldb_key.cc

e0fa3d7

var-const approved these changes Mar 21, 2018

View reviewed changes

wilhuff added 2 commits March 21, 2018 18:00

Implement review feedback

4c27c83

More review feedback

e5cffe2

wilhuff force-pushed the wilhuff/leveldb-key branch from 3b40fa7 to e5cffe2 Compare March 22, 2018 01:00

wilhuff merged commit cf630bf into master Mar 22, 2018

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

Port FSTLevelDBKey to C++ (firebase#937)

af05bd1

This was referenced Aug 5, 2018

Migrate [LevelDBKey descriptionForKey:] to DescribeKey() #1638

Merged

Migrate LevelDB Key classes to C++ #1651

Merged

paulb777 deleted the wilhuff/leveldb-key branch August 8, 2018 17:08

firebase locked and limited conversation to collaborators Nov 6, 2019

Port FSTLevelDBKey to C++ #937

Port FSTLevelDBKey to C++ #937

Uh oh!

Conversation

wilhuff commented Mar 16, 2018

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

var-const Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

var-const Mar 19, 2018 •

edited

Loading