Fix Dimension Serialization Problem with Nested Queries

[garyluoex]

• Modified DimensionToDefaultDimensionSpec serializer to serialize dimension to apiName if it is not the inner most query
• Added helper hasInnerQuery to Util in serializer package to determine if current query is the inner most query or not
• Added tests for DimensionToDefaultDimensionSpec

[cdeszaq]

Can we extract the "tree walk to find the next level up" into it's own method? Both of the helper methods in this class are currently doing that work. Optional<DruidQuery> may be a useful return type to use, since it may let us keep things clean.

Here's one (untested) idea:

public <T> Optional<T> withContainingQuery(JsonGenerator gen, Function<DruidQuery, T> success) {
    JsonStreamContext context = gen.getOutputContext();
    while (context != null) {
        Object parent = context.getCurrentValue();
        if (parent instanceof DruidQuery) {
            return Optional.of(success.apply((DruidQuery) parent));
        }
        context = context.getParent();
    }
    return Optional.empty();
}

[garyluoex]

I am amazed by how much line reduction this implementation provides, however I am a bit worried about the readability reduction associated with it.

[archolewa]

I don't see how this reduces readability any. Personally, I generally find it more readable when things are broken out into helper methods, especially if there is duplication.

First, the helper methods abstract away some of the "how" and replace it with "what." I find this good for two reasons: One, I generally find the "what" more interesting than the "how" when I'm starting to dig into some code. Two, I generally find it easier to understand the "how" when I already know the "what," then I do to derive the "what" from the "how."

Second, this makes it immediately clear that this code is identical both here and in the other helper method. Without this method, I would have to read the two code blocks very closely in order to confirm that they are doing the same thing (as opposed to very subtly different things). That in and of itself is a huge readability bonus.

Though the success variable could probably stand to be renamed. Perhaps something like queryTransform? I'd have to dig in a bit more to get enough context to provide a better suggestion.

[cdeszaq]

This looks like it was mistakenly pulled in. I think this is already on master, right?

[cdeszaq]

This can actually all be unwrapped since it can fit on 1 line now

[michael-mclawhorn]

Javadoc is incorrect. This query returns the mapped value of the druid query under the success function if the current context if the current context is a druid query.

[cdeszaq]

👍

[michael-mclawhorn]

👍 Once the build passes.

[michael-mclawhorn]

Build is failing on one of the tests.

[cdeszaq]

I'm pretty sure the build is failing due to load during tests caused by a too-tight wait period. Since nothing else relevant to this PR is failing, marking this as mergeable.