Hi @MattSkala. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

@wilhuff If I'm reading the relevant code correctly, it uses ConcurrentHashMap, which is thread-safe, but also relies on reassigning the variable itself, which isn't. One alternative that comes to mind is creating the map non-lazily and clearing it instead of setting it to null. Let me know what you think.

/ok-to-test

@var-const I agree making fieldValueCache non-null is a better solution, should I change it?

ConcurrentHashMap is pretty expensive and most users get the whole document contents rather than just an individual field. The fieldValueCache is lazily instantiated to avoid adding this overhead to every Document unconditionally. The cache exists at all because internally, when sorting results, we can access the same field multiple times. (The TODO in the code is really indicating that we should apply a Schwartzian transform before sorting at which point we can eliminate the cache altogether.)

This fix seems OK as is.