Value.isNull slightly non-intuitive in stream mode

[Yuval Shavit]

In stream mode, Value.isNull() never advances to the next object. When isNull() == false, this seems perfectly reasonable; it lets you do a pattern such as:

    if (!value.isNull) {
        int intValue = value.getInt();
        doSomething( intValue );
    }

But when isNull() returns true, this behavior can be a bit counter-intuitive; we probably want to skip to the next item. This is a tiny bit awkward, but worse, it's not obvious we need it. Consider this code:

    StringBuilder sb = new StringBuilder();
    for(int i = 0; i < segments; ++i) {
        if (!value.isNull()) {
            int intVal = persistitValue.getInt();
            sb.append(intVal);
        }
        else {
            sb.append("<null>");
        }
    }

It looks reasonable, but it's broken; after we hit the first null value, we'll not advance to any more values, and every subsequent segment will look like null. The solution is:

        ...
        else {
            sb.append("<null>");
            persistitValue.skip();
        }

I'm not sure what to do about this. One solution would be to keep it as-is, but note this use case in the javadoc. Another would be to add an argument to isNull, a boolean specifying whether we should skip to the next segment if this one is null. I would prefer that one, as it forces users to learn about this problem and declare how they want to solve it.

[Peter Beaman (pbeaman)]

I think I would prefer isNull() not to advance the cursor. I know it's a little inconvenient to add a call to skip() but I think that's the path of least surprise. It would be especially surprising, I believe, for the method to conditionally advance the cursor. I would be more than happy to add documentation on this to help hapless hackers avoid the anomaly.

I agree that in your code pattern the result of not knowing to call skip (or getNull()) causes a big surprise too, but if the behavior is properly documented I think others won't fall into the same trap.

Note that this is a problem caused by use of primitives in Java. The get() method that returns an object cleanly returns either the object or null and advances the cursor. If there were only objects, then getInt() could return an Integer or a null. Perhaps that's the ultimate solution.

[Nathan Williams (nwilliams)]

There are something like 120 public methods on Value, 95 of which deal with the objects contained in it (put, get, isNull, etc). A good chunk of those, are as Peter mentions, dealing with primitives explicitly. I don't foresee those going away any time soon, though, given Persistit's focus on performance and what we know about GC.

It would be nice to make the API as use friendly as possible. Once a user knows about stream mode, I think all of the put and get methods are pretty obvious. That leaves decodeDisplyable, isType, getCursor, setCursor, isNull, getType, hasMoreItems, and skip (I think).

Some of these don't document their stream behavior (decodeDisplyable, isType, getCursor, setCursor, isNull, getType), some of them don't properly handle stream and error conditions (isType, isNull, getType) and some are not defensive enough outside of stream (hasMoreItems).

After more thought, I think I'm of the opinion that isNull() should not take a flag but that all the "query only" methods should be documented, and fixed, to act only on the current position in stream mode.

[Yuval Shavit (yshavit)]

I'm actually not familiar with get/setCursor. Bug I think what's different about isNull() as compared to the others is that it leads you to a binary path where one branch advances the cursor "naturally" (some personal-preference handwaving here), and the other doesn't -- you need to "manually" push it along.

I'm guessing (I don't have the javadoc handy right now) that getType() returns null or Void if the value is null? If so, that would also be a case where this sort of unexpected behavior would pop up.

As for docs, I'm of the opinion that most people (who's got two thumbs and includes himself in this list?) look at the IDE's autocomplete and maybe the first sentence of the javadoc for about 60% of their learning about new APIs. Putting the implications of stream mode below the fold wouldn't, I think, alert most users to the situation -- and it's not something that pops up when reading the code over when debugging. My suggestion about an arg is essentially a way to force users to read up about this behavior -- and the fact that the simple/unconditional behavior is probably *not* what they want in many cases (such as the ones in the original description).

Let me amend my proposed solution slightly, and see if it's more palatable to either of you:

    enum StreamRead {
        ADVANCE_ON_NULL, NO_ADVANCE_ON_NULL
    }

    boolean isNull() {
        return isNull(StreamRead.NO_ADVANCE_ON_NULL);
    }

    boolean isNull(StreamRead readMode) {
        ...
    }

[Mike McMahon (mmcm)]

I don't propose changing anything, but in my opinion having a "stream mode" is all around a bit confusing. Personally, I would feel more comfortable with getXXX / putXXX operations and readXXX / writeXXX operations, differing in whether they advance, or even separate stream objects.

[Peter Beaman (pbeaman)]

We left isNull() unchanged in com.persistit.Key and com.persistit.Value. But we also added

  Key#isNull(boolean skipNull)
  Value#isNull(boolean skipNull)

These new methods enable skip-on-null if the skipNull parameter is true. isNull) is equivalent to isNull(false) for both classes.