CASSANDRA-19932: Allow to define extensions while creating table

[lukasz-antoniak]

Fixes CASSANDRA-19932.

[absurdfarce]

A few things to look into here but overall this looks pretty good!

[absurdfarce]

I'd also prefer to isolate the logic for mapping bytes to strings in the context of the generated CQL within this class. Rather than modifying OptionsUtils to make changes which could affect the handling of any option we should constrain the bytes-to-hex string encoding we're looking for here to this method:

  /** Attaches custom metadata to CQL table definition. */
  @NonNull
  @CheckReturnValue
  default SelfT withExtensions(@NonNull Map<String, byte[]> extensions) {
    return withOption("extensions", Maps.transformValues(extensions, ByteUtils::toHexString));
  }

[lukasz-antoniak]

This will not work, as then extension will be rendered as {'ext1': '0x76616c31'} (hex value enclosed with quotes, so string), instead of {'ext1': 0x76616c31}.

[absurdfarce]

That's correct, but that's driven by a quirk regarding the encoding of Strings. OptionsUtils.extractOptionValue() just blindly assumes that any string passed in should be understood as a CQL string (and thus should be quoted). We can work around this by any number of methods. Pretty much any type other than a String (or String subtype) would get what we want, so for example this actually works fine:

 /** Attaches custom metadata to CQL table definition. */
  @NonNull
  @CheckReturnValue
  default SelfT withExtensions(@NonNull Map<String, byte[]> extensions) {
    return withOption("extensions", Maps.transformValues(extensions, (s) -> CharBuffer.wrap(ByteUtils.toHexString(s))));
  }

But the above code is still relying on details of the implementation (the fact that CharBuffer isn't a String) to do what it needs to do... it's not clear to someone reading the code why we're doing that.

We could modify extractOptionValue() to take an arg governing whether strings should be quoted or not but then that arg would have to be pushed down through the call to OptionsUtils.buildOptions() in DefaultCreateTable... which isn't exactly intuitive either.

A far better approach would be to rely on the type system:

  /**
   * Wrapper class to indicate that the contained String value should be understood to represent
   * a CQL literal that can be included directly in a CQL statement (i.e. without escaping).
   */
  class CqlLiteral {

    private final String val;
    public CqlLiteral(String val) {
      this.val = val;
    }

    public String toString() { return this.val; }
  }
...
  /** Attaches custom metadata to CQL table definition. */
  @NonNull
  @CheckReturnValue
  default SelfT withExtensions(@NonNull Map<String, byte[]> extensions) {
    return withOption("extensions", Maps.transformValues(extensions, (s) -> new CqlLiteral(ByteUtils.toHexString(s))));
  }

This also makes the test pass and is much clearer with respect to our intent.

Finally, I'd note that the question of making these quotes come out okay is very different from the question of whether we should encapsulate the logic in RelationOptions.withExtensions() or whether we should force changes to OptionsUtils to make it work. If we believe that this encapsulation is useful we should seek out one or more options to the encoding question (as discussed above) and not scrap the idea in it's entirety.

[lukasz-antoniak]

I have tried to make CqlLiteral more reusable. Can you review now?

[absurdfarce]

I very much like your idea of creating CqlLiteral as a standalone class @lukasz-antoniak. But as I was looking this over again earlier today I started wondering about a few things. First off: I started worrying if CqlLiteral sitting right next to the default impl of the Literal interface (DefaultLiteral) would be confusing; they have very similar names but don't really have anything in common. That in turn got me wondering... is there some way we can do this with just Literal (via QueryBuilder.literal())? We have to tweak a few things; OptionsUtils relies entirely on StringBuilder.append() which means DefaultLiteral needs a good toString() (it doesn't define it's own currently) but if we get the right thing there DefaultLiteral is also not an instance of String so (hypothetically) OptionsUtils.extractOptionValue() won't quote it.

I haven't been able to make this work yet but I wanna muck around with it a bit more before I write off the idea.

[absurdfarce]

Yup, this looks workable to me. Nice work @lukasz-antoniak!

[absurdfarce]

Excellent, thank you @clohfink! With the extra 👍 we're good to go on this one... @lukasz-antoniak can you squash these commits down and add the standard commit message?