fix: remove any rowtime or rowkey columns from query schema (MINOR) (Fixes 3039)

[big-andy-coates]

Description

Fixes #3039

Remove any ROWTIME or ROWKEY columns from query schema

These columns can be copied into the source schema by DataSourceNode.

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

• Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
• Ensure relevant issues are linked (description should include text like "Fixes #")

[agavra]

Thanks for the quick turn around on this one @big-andy-coates!

[hjafarpour]

LGTM.

[agavra]

After playing around with this change a little more, I think what makes more sense for transient queries is to remove the key and meta schemas entirely, and keep the system fields in the values. This is because we only return the value fields for transient queries.

I made this change:

  public LogicalSchema withoutMetaAndKeyFields() {
    return new LogicalSchema(
        SchemaBuilder.struct().build(),
        SchemaBuilder.struct().build(),
        valueSchema,
        alias
    );
  }

After that, EXPLAIN does this:

ksql> EXPLAIN SELECT ID FROM FOO;

ID                   :
SQL                  : SELECT ID FROM FOO;

 Field | Type
-------------------------
 ID    | VARCHAR(STRING)

ksql> SELECT ID FROM FOO;
1

ksql> EXPLAIN SELECT * FROM FOO;

ID                   :
SQL                  : SELECT * FROM FOO;

 Field   | Type
-------------------------------------
 ROWTIME | BIGINT           (system)
 ROWKEY  | VARCHAR(STRING)  (system)
 ID      | VARCHAR(STRING)
-------------------------------------

ksql> SELECT * FROM FOO;
1562021756897 | a | 1

To me, this makes more sense. Either we do that, or we always return ROWKEY/ROWTIME for transient queries in the data as well. Thoughts? (cc @hjafarpour )

[big-andy-coates]

Man this adding/removing ROWKEY and ROWTIME is a pain.

If transient queries only return value fields, then yes we should only include the value schema in the EXPLAIN plan - I'll take a longer look tomorrow.

[hjafarpour]

I agree. What @agavra mentioned is the same as the current behavior and makes sense to me too.

[agavra]

Thanks @big-andy-coates!