implement PartialOrd for days_ms and months_days_ns types

[houqp]

No description provided.

[jorgecarleitao]

Thanks a lot, @houqp ! Unfortunately, intervals do not seem to have a natural order (at least in Java and C++). See this comment from @emkornfield, which led me to remove the PartialOrd from both interval types.

[houqp]

Interesting, that's a good call. It looks like days_ms should have an order though right?

On that note, this is causing problem in datafusion because ScalarValue now implements PartialOrd. I am going think about what's the best way to handle it.

[emkornfield]

it is still unclear to me if day_ms is supposed to have ms normalized to less then a day (in which case partial order makes sense). but month, day, nanos do not based on how we defined them.

[houqp]

@jorgecarleitao how about we implement partial_cmp for months days manually to always return None?

[jorgecarleitao]

Good idea on using the Option aspect here.

If ns is less than a day and days is less than 28 we do have an order; else we return None (and equivalent for day_ms)?

[houqp]

@emkornfield good point on the ms part. After thinking more on this, I think we can define a better partial_cmp implementation for both types instead of just always return None.

For months_days, we can check for the days component, if it's less than 28, then it's safe to return an order.

[houqp]

ha, never mind, @jorgecarleitao beat me to it :D

[houqp]

I think this is exactly what partial order is for, return no order when there is ambiguity.

[jorgecarleitao]

exactly, great idea, @houqp with the None!