Core: View metadata implementation

[nastra]

This is a continuation PR of #6559 and I've addressed all remaining comments.
@stevenzwu @jackye1995 @rdblue since you guys were reviewing the original PR, I've added you here as well. Please take a look when you can.

[ConeyLiu]

@nastra Is it possible to abstract a common interface for table and view because they have very similar methods?

[amogh-jahagirdar]

Overall looks great to me just some minor remaining comments, and should be good to merge. Thanks a ton for carrying this forward @nastra! I'll reply on the original PR and we can probably close that one.

[nastra]

@nastra Is it possible to abstract a common interface for table and view because they have very similar methods?

@ConeyLiu I don't think this is desirable, because ViewMetadata and TableMetadata only overlap in a few fields, so adding an abstract common interface might add unnecessary complexity

[ConeyLiu]

@nastra Thanks for your response, it indeed makes the code more complex.

[rdblue]

I think this should be Derived rather than Lazy. Lazy makes it more complicated and there's no real benefit to not just calculating this at construction.

[nastra]

generally I agree with you that @Value.Derived is the better option.
However, given that we're deriving from a collection, it makes it difficult/impossible to do validation on the collection before eager validation on the derived field kicks in.
See also my comment in #7759 (comment) about default collection behavior and why it's better to have @Value.Lazy here

[rdblue]

In that case, does it make sense to make this Lazy at all? They're not lazy or derived in TableMetadata:

  public Schema schema() {
    return schemasById.get(currentSchemaId);
  }

[nastra]

I'm ok removing @Value.Lazy here, since the only thing it buys us is that it doesn't re-compute the value from the underlying hash map, which is O(1) anyway

[rdblue]

I think this is confusing. The builder should enforce the maximum number of versions, not the check method. It's awkward that this returns a copy of the view metadata.

[nastra]

this seems to be the "official" way of normalizing an object: http://immutables.github.io/immutable.html#normalization

Not sure it's worth adding our own builder class for this particular case

[rdblue]

This is strange to me and I'm not a big fan of having a checkAndNormalize in the interface, but okay I guess? It doesn't seem concerning enough to block using Immutables but it is annoying that the pattern requires exposing additional methods that don't have a clear contract (like validate).

[nastra]

I agree here completely.
It would be great if there could be a better & shorter alternative to achieve this.
The (longer) alternative would be to have our own builder that internally uses ImmutableViewMetadata.builder() and does the "normalization" but that doesn't prevent anyone from using ImmutableViewMetadata.builder() directly

[rdblue]

Yeah, but if we're using our own builder then what's the point of immutables?

For now, I think we should just get this in to unblock the next PR. We should follow up with the API that adds a new view representation and version. If that goes smoothly then it will be fine. If it doesn't fit then we can remove immutables and go with a direct implementation.

[rdblue]

Aren't schemas and versions required to be non-null and non-empty?

[nastra]

Collections are empty by default if not set and typically they are being checked in a @Value.Check method. See also immutables/immutables#1429 (comment) for some historical reasons why Collections are being empty by default if not set.

The other alternative would be to use the Java Bean Validation API, which would allow expressing validations on Collections via annotations, but I don't think we'd want to do that here.

That's the main reason why I'd like to keep currentVersion() and schema() being @Value.Lazy so that we don't have misleading error messaging.
What I mean here is that for example, if currentVersionId() is set but versions() is empty, we would see java.lang.NullPointerException: currentVersion (because currentVersion cannot be derived from versions() and thus ends up being null) rather than java.lang.IllegalArgumentException: Cannot find current version 23 in view versions: [1]

[rdblue]

If this validation fails, won't it cause the view to be broken and unfixable? We won't be able to construct the ViewMetadata so there would be no way to fix this if another implementation sets it to -1 or something for unlimited history. We would either need to document this requirement in the spec or fail more gracefully.

[nastra]

yes that is true, one wouldn't be able to construct the ViewMetadata object`, because it's technically failing validation.
For keeping unlimited history I think we have the following options:

1. allow -1 to indicate unlimited (not sure if we have any other properties that carry such semantics).
2. let users use a large value with Integer.MAX_VALUE being the maximum. This seems to be more appropriate IMO, since the view spec mentions The number of versions to retain is controlled by the table property: version.history.num-entries.. However, I can also see the argument to "which value is large enough?".

Whatever we think carries the right semantics to indicate unlimited history should go into the validation logic + the spec.

[rdblue]

I think I would rather not fail construction because of this. If the value is invalid, then let's just ignore it, warn, and not change the versions list.

[nastra]

seems reasonable, I've updated this to issue a WARN and not modify the history

[rdblue]

This looks about ready, although there are some minor comments.

[rdblue]

Thanks, @nastra! And thanks to @amogh-jahagirdar and @jzhuge for major parts of this as well!