Add Compare to GroupByScalar

[tustvold]

Whilst looking to implement #141 I couldn't work out a way to compare ScalarValue types.

There is, however, GroupByScalar which seems pretty compelling:

• It lacks an internal Option, allowing the caller to handle null-ordering how it deems fit
• It uses OrderedFloat, establishing a standards compliant ordering of floats
• It only provides the value variants that make sense to compare
• The plumbing to convert from a ScalarValue already exists

It therefore seems to be what I'm looking for, however, it doesn't currently have a PartialOrd or Ord implementation and is crate-local...

Based on this I have a couple of questions:

• Is there some other mechanism I should use to compare fields across record batches (at potentially different row offsets)
• Can we add #[derive(PartialOrd, Ord)] to GroupByScalar - note this would establish an ordering based on the order of enum variants between unrelated variants
• Can we expose GroupByScalar publicly, potentially renamed to something to reflect its more generic utility, perhaps ComparableScalar or something

[alamb]

Perhaps @Dandandan or @jorgecarleitao or @jhorstmann have some thoughts in this area

[alamb]

I have also often wondered "why do we have GroupByScalar at all" (aka why can't we just use ScalarValue)

[jorgecarleitao]

I have also often wondered "why do we have GroupByScalar at all" (aka why can't we just use ScalarValue)

I agree.

AFAIK it is a historical artifact. First it came the GroupByScalar, when the group by was first implemented; then the ScalarValue emerged to nullable uniformise scalar operations, particularly aggregations.

[alamb]

So maybe we can add compare to gby scalar (or ScalarValue) as part of this PR and then unify the two (remove GroupByScalar --> ScalarValue) as a follow on PR

[tustvold]

Would we change ScalarValue to store OrderedFloat and add Eq in addition to PartialOrd and Ord or keep using raw floats and only define PartialOrd?

[alamb]

Would we change ScalarValue to store OrderedFloat and add Eq in addition to PartialOrd and Ord or keep using raw floats and only define PartialOrd?

I would suggest we don't change ScalarValue in this PR and use GroupByScalar,

When we get to unifying GroupByScalar and ScalarValue I would imagine we would have to use ordered_float in ScalarValue

[Dandandan]

I agree combining them makes a lot of sense 👍

In aggregates I was also stumbling on some strange issues relating to evaluation of scalars while working on this: #339

I think when we have a bit more unified code it makes it easier to solve these kind of issues.