search

substrait-io / substrait-java

Apache License 2.0
72 stars 70 forks source link

feat: add POJOs for ConsistentPartitionWindowRel and WindowRelFunction #231

Closed bvolpato closed 4 months ago

bvolpato commented 4 months ago

Adding the POJO for ConsistentPartitionWindowRel and WindowRelFunction: https://github.com/substrait-io/substrait/blob/main/proto/substrait/algebra.proto#L233-L267

I wasn't sure about the organization/place of some parts since it's not physically under Expression, but there's a lot in common, even highlighted in the proto:

// This message mirrors the WindowFunction message but removes the fields defining the partition, // sorts, and bounds, since those must be consistent across the various functions in this rel. Refer // to the WindowFunction message for a description of these fields.

So I tried to keep it as a new base class, but tried to keep the converters as close as they were to make it more concise + simplifying reuse.

--

I'll have an Isthmus change following (this is essentially a cleanup / split of https://github.com/substrait-io/substrait-java/pull/228), and @vbarua rightfully suggested breaking it down.

bvolpato commented 4 months ago

Thanks for the great review @vbarua 💯.

All good points! Pushed https://github.com/substrait-io/substrait-java/pull/231/commits/1bb98364ccaf93271f506a06d959eb73a8d3b1d1 addressing them.