feat: add Arrow and Orc file formats

[westonpace]

BREAKING CHANGE: The substrait/ReadRel/LocalFiles/format field is deprecated. Newer consumers should intrepet older files as parquet. Older consumers should interpret newer files as FILE_FORMAT_UNSPECIFIED.

[jvanstraten]

Rather than adding to the FileFormat enum, would it perhaps be more sensible to replace the enum with a oneof? Something like

message FileOrFiles {
  oneof path_type {
    ...
  }

  // Original file format enum, superseded by the file_format oneof.
  reserved 5;
  reserved "format";

  // the index of the partition this item belongs to
  uint64 partition_index = 6;

  // the start position in byte to read from this item
  uint64 start = 7;

  // the length in byte to read from this item
  uint64 length = 8;

  oneof file_format {
    ParquetFormat parquet = 9;
    ArrowIpcFormat arrow_ipc = 10;
    google.protobuf.Any extension = 31;
  }
  
  message ParquetFormat {
  }
  
  message ArrowIpcFormat {
    ArrowIpcSubformat subformat = 1;
    
    enum ArrowIpcSubformat {
      ARROW_IPC_SUBFORMAT_UNSPECIFIED = 0;
      ARROW_IPC_SUBFORMAT_STREAM = 1;
      ARROW_IPC_SUBFORMAT_FILE = 2;
    }
  }
}

File formats like CSV, for example, will really need more information than just "this is a CSV file" to be correctly parsed if they are added at some point. Currently the enum only has one valid value, so it's better for backwards and forwards compatibility to think about this now than later.

ETA: looks like this also came up in #138. Also added extension to the oneof variants.

[westonpace]

I think your proposal is fine. That was originally kind of what I was aiming for with #138 but the response I got there seemed to suggest that "having extra stuff" is a bit of a special case and we will just use the extension for that. I don't really have any strong opinion whether we create dedicated messages or not for the simple self-describing protocols. So, barring any further input, I will adopt your suggestion and update my PR in a few days.

[jvanstraten]

There can also just be a catch-all "empty message" type somewhere. That's what I did for the validator output format. Then you can also do

oneof file_format {
  Empty parquet = 9;
  ArrowIpcFormat arrow_ipc = 10;
  google.protobuf.Any extension = 31;
}

to save some clutter. The oneof variant is determined by the field index, so I'm pretty sure it doesn't matter that the message types would be the same if more self-describing formats are added.

In fact, I wouldn't be surprised if protobuf defines an empty message type in its standard library. I just never bothered to look for one, since it's so easy to define your own and it doesn't cost anything (well, maybe a tiny amount of compile time I suppose).

The data could indeed be attached via an advanced extension too, but it'd be rather awkward with the current structure. The nearest extension field is in LocalFiles, which is common to all FileOrFiles elements, while each FileOrFiles message has its own format field. Then again, it probably doesn't make sense to specify both a CSV file and a Parquet file or whatever in a single read relation, but then I'd argue that the file format should have been part of LocalFiles rather than FileOrFiles. It's not really clear to me why the structure was originally defined this way.

[jacques-n]

I'm very supportive of a change like what @jvanstraten is suggesting above. I agree that many formats need specific properties.

I think the less supportive part of my response in #138 was the proposal of something like a string with opaque consumption. We should work to try to keep things well specified (or use the any object at whatever level is appropriate).

[westonpace]

I updated this PR to a form similar to what @jvanstraten suggested. I dropped the "arrow streaming" format option since that doesn't make sense for local files anyways. I added Orc inspired by #202 .

That being said, I'm not so certain we need to worry about reserving 5/format given the early stage and the lack of support for backwards compatibility here anyways.

[cpcloud]

@jacques-n Perhaps we should disable the buf + BREAKING CHANGE detection in CI until we've hit 1.0?

[jacques-n]

I suggest we create empty message types for each of these. That would make more sense to people, I think. E.g. message ParquetFormat {}

Let's update format to be marked as deprecated rather than deleting (for now). We can batch up removal of deprecations when we want to bump the format version.

[westonpace]

I suggest we create empty message types for each of these. That would make more sense to people, I think. E.g. message ParquetFormat {}

Done. Please check this to make sure I understood your point.

Let's update format to be marked as deprecated rather than deleting (for now). We can batch up removal of deprecations when we want to bump the format version.

I was hoping that "breaking changes == minor" means that we no longer need to worry about batching breaking changes. Let me know if something further is needed here.

[jacques-n]

#210 is now merged. @westonpace and @jvanstraten, have you two come to a consensus on this? Getting this merged should also help us get some solution for #174 and #202 merged as well.

[westonpace]

I think the conensus was "yes, we need to reserve fields". For this PR in particular on the question of "do we need a legacy option?" I think the answer is "no".

So the last remaining question is:

Can we garbage collect old (now-unused) enums?

This PR does deprecate the FileFormat enum. I believe there was a bit of discussion on Slack here: https://substrait.slack.com/archives/C02D7CTQXHD/p1653693433051879

My interpretation of @jvanstraten 's was that yes, we can garbage collect unused enums. But I'm not entirely sure if we're thinking of the same definition of enum (e.g. protobuf enum vs substrait enum). So I'd appreciate one more review from @jvanstraten on this PR.

I'll update the title / description / commits to semantic release commits.

[westonpace]

Actually, as I was updating the commits, I also added a new default behavior in the comments to handle the case where no oneof is specified.

In addition I addressed Jacques point above, adding empty message types instead of using google.protobuf.Empty.

[jacques-n]

Yes, this captures my request. Thanks. Will wait on @jvanstraten confirmation of agreement to merge.

[jvanstraten]

My interpretation of @jvanstraten 's was that yes, we can garbage collect unused enums. But I'm not entirely sure if we're thinking of the same definition of enum (e.g. protobuf enum vs substrait enum).

Oh. Based on this context I guess we weren't, but I don't see why we shouldn't remove things that are no longer reachable.

[jvanstraten]

LGTM other than that default behavior.

ETA: pfft, I guess I didn't submit the change request. w.r.t. the default behavior comment: a oneof should never have default behavior other than to reject the plan. With the suggested behavior, if we add a new option later and pass a plan with that option to an old consumer, it would silently incorrectly default to parquet. We'd never be able to add to the oneof without imposing a (semantic) breaking change.

[westonpace]

ETA: pfft, I guess I didn't submit the change request. w.r.t. the default behavior comment: a oneof should never have default behavior other than to reject the plan. With the suggested behavior, if we add a new option later and pass a plan with that option to an old consumer, it would silently incorrectly default to parquet. We'd never be able to add to the oneof without imposing a (semantic) breaking change.

I wish you weren't right but it looks like you are. This will completely break backwards compatibility then.

[westonpace]

In the future I suppose we could avoid the break by keeping the old field and changing the enum...

      enum FileFormat {
        FILE_FORMAT_UNSPECIFIED = 0;
        FILE_FORMAT_PARQUET = 1;
        FILE_FORMAT_FUTURE = 2;
      }  

Consumers would then only check the oneof if FILE_FORMAT_FUTURE

But I do not think that change is justified at this point.