ARROW-12277: [Rust][DataFusion] Implement and test Count/Min/Max aggregates for Timestamp(_,_)

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.

[alamb]

Rationale:

If you try and aggregate (via MIN, for example) a column of a timestamp type, DataFusion generates an error:

Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.

For example, from IOx

> show columns from t;
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| table_catalog | table_schema | table_name | column_name | data_type | is_nullable |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
| datafusion | public | t | a | Utf8 | NO |
| datafusion | public | t | b | Timestamp(Nanosecond, None) | NO |
+---------------+--------------+------------+-------------+-----------------------------+-------------+
2 row in set. Query took 0 seconds.
> select sum(b) from t;
Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.")

Changes:

Add support for aggregating timestamp types and tests for same

Notes

Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to ScalarValue)

Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12277

[alamb]

I added support for TimestampSecond here and renamed ScalarValue::TimeMillisecond --> ScalarValue::TimestampMillisecond so that the ScalarValue enum type matches up with the DataType enum type name

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[alamb]

This error message is pretty horrible. I filed https://issues.apache.org/jira/browse/ARROW-12319 to track improving it

[velvia]

Agreed, thanks for filing a ticket!

[alamb]

FYI @Dandandan / @returnString

[returnString]

I'd just hit up against this exact issue this week with some silly SQL clients doing timestamp queries on boot - nice timing! Code looks good to me 👍

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?
It looks like PostgreSQL doesn't support it: https://www.postgresql.org/docs/13/functions-aggregate.html

[velvia]

That's a good question. I can think of great uses for avg, and difference (latency between two timestamps), but not really for sum, other than that sum is an important part of computing average?

[Dandandan]

Hey @alamb this is looking good.
I'm wondering whether we should support sum/avg for timestamp?

[alamb]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

[Dandandan]

What are the exact use cases for summing timestamps? When does it make sense?

@Dandandan that is an excellent question. I will freely admit I was just heads down trying to get all the aggregates working rather than thinking if I should be working. I can't think of any time that sum(timestamp) really makes sense to be honest.

Do you think I should remove support for summing timestamps?

As for AVG, I added a note to https://issues.apache.org/jira/browse/ARROW-12318 with your observation. 🤔 good question

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

will do

[alamb]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

[returnString]

Thinking more about this: do we need to be careful about numeric limits? E.g. the current implementation of AvgAccumulator in the physical plan tracks the whole sum and count, and does sum / count at the end, meaning we might hit i64::MAX in the sum component given enough entries in an avg input with a sufficiently high-precision timestamp type.

Edit: don't mind me, it forces an f64 accumulator - I saw ScalarValue and panicked :)

[Dandandan]

At least would be best to remove support for sum, as we don't have an absolute zero for timestamps. For average, I think it may make some more sense, but I think the use case seems limited to me.

@Dandandan -- I just double checked and the IOx project actually has need of Sum (and Avg) for timestamps due to the semantics of the Flux and InfluxQL languages. I can implement this functionality for custom user defined aggregates if needed, but I would prefer having the functionality in DataFusion directly so that anyone else can potentially use it too

I don't really understand the comment about "absolute zero" for timestamps.

Would it be ok with you if I merged in support for Sum for timestamps?

My reaction to enable sum for timestamps was that it depends on the starting/zero value for the value, in this case the UNIX epoch.
The result of a addition/sum on timestamp is meaningless as it depends on the choice of the value of zero (1980 + 1980 + 1980 = 2000!?, 1970 + 1970 = 1970!?, 1960 + 1960 = 1950!?). When we would change the zero value (to Jan 1 1900 for example), we would totally change the meaning of adding timestamps.

If there was an "actual" zero date (like Kelvin for example) summing the values makes sense but there isn't such a thing for timestamps of course.

[alamb]

I have removed SUM from this PR and will figure out what IOx is doing that seems to require sum and doesn't make sense. Thanks @Dandandan

[Dandandan]

Thanks 👍 looks good. Let's see what we should do with sum later 🙂

[velvia]

Great PR @alamb !

[velvia]

Testing string formatting seems kinda yucky and prone to fail when the formatting changes. Is there a better way to test the output here?

[alamb]

Thanks @velvia --In general I agree that checking string representations in tests is brittle when formatting changes.

In the case of sql / tabular output, I think it is the best alternative I have seen because:

1. The output formatting does not change often
2. Updating these tests are easy (simply copy/paste the output of the test failure into the test after verifying it)
3. These tests follow the same pattern as the rest of the tests in this module

[velvia]

Min and max definitely makes sense.

[velvia]

👍 Great to be consistent.
Not related to this PR necessarily but I'm going to work on support for converting date strings to timestamps of different resolutions. Right now only Nanos are supported, ideally we'd have to_timestamp(...) that allows choosing nanos, micros, millis, or seconds under the hood.

[velvia]

👍 this will be really useful.