alamb commented on code in PR #8193:
URL: https://github.com/apache/arrow-datafusion/pull/8193#discussion_r1401920583
##########
datafusion/sql/tests/sql_integration.rs:
##########
@@ -597,20 +597,14 @@ fn select_neg_filter() {
fn select_compound_filter() {
let sql = "SELECT id, first_name, last_name \
FROM person WHERE state = 'CO' AND age >= 21 AND age <= 65";
- let expected = "Projection: person.id, person.first_name, person.last_name\
- \n Filter: person.state = Utf8(\"CO\") AND person.age >=
Int64(21) AND person.age <= Int64(65)\
- \n TableScan: person";
+ let expected = "Projection: person.id, person.first_name,
person.last_name\n Filter: person.state = Utf8(\"CO\") AND person.age >=
Int64(21) AND person.age <= Int64(65)\n TableScan: person";
Review Comment:
I think it would be good to revert these changes or reformat them if there
is a difference, to make the diff clearer
##########
datafusion/sqllogictest/test_files/timestamps.slt:
##########
@@ -1788,8 +1788,46 @@ SELECT TIMESTAMPTZ '2020-01-01 00:00:00Z' = TIMESTAMP
'2020-01-01'
----
true
-# verify to_timestamp edge cases to be in sync with postgresql
-query PPPPP
-SELECT to_timestamp(null), to_timestamp(-62125747200), to_timestamp(0),
to_timestamp(1926632005177), to_timestamp(1926632005)
+# verify timestamp cast with integer input
+query PPPPPP
+SELECT to_timestamp(null), to_timestamp(0), to_timestamp(1926632005),
to_timestamp(1), to_timestamp(-1), to_timestamp(0-1)
----
-NULL 0001-04-25T00:00:00 1970-01-01T00:00:00 +63022-07-16T12:59:37
2031-01-19T23:33:25
+NULL 1970-01-01T00:00:00 2031-01-19T23:33:25 1970-01-01T00:00:01
1969-12-31T23:59:59 1969-12-31T23:59:59
+
+# verify timestamp cast with integer input timestamp literal syntax
+query PPPPPP
+SELECT null::timestamp, 0::timestamp, 1926632005::timestamp, 1::timestamp,
-1::timestamp, (0-1)::timestamp
Review Comment:
Another way to make the intent of the test clearer and ensure these values
don't diverge would be to do something like:
```sql
SELECT
null::timestamp = to_timestamp(null),
0::timestamp = to_timestamp(0),
1926632005::timestamp = to_timestamp(1926632005),
...
```
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -224,14 +225,26 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLExpr::Cast {
expr, data_type, ..
- } => Ok(Expr::Cast(Cast::new(
- Box::new(self.sql_expr_to_logical_expr(
- *expr,
- schema,
- planner_context,
- )?),
- self.convert_data_type(&data_type)?,
- ))),
+ } => {
+ let dt = self.convert_data_type(&data_type)?;
+ let expr =
+ self.sql_expr_to_logical_expr(*expr, schema,
planner_context)?;
+
+ // int/floats input should be treated as seconds rather as
nanoseconds
Review Comment:
I was worried about this change as it handles the case of `CAST (x to
TIMESTAMP)` but I was worried that it would be inconsistent with automatic
coercion rules for numbers.
Then I tried some examples and I think there are no automatic coercion rules
for integers to timestamps, which makes sense:
```
❯ select now() - 100000;
Error during planning: Cannot coerce arithmetic expression
Timestamp(Nanosecond, Some("+00:00")) - Int64 to valid types
❯ select now() > 100000;
Error during planning: Cannot infer common argument type for comparison
operation Timestamp(Nanosecond, Some("+00:00")) > Int64
```
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -224,14 +225,26 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLExpr::Cast {
expr, data_type, ..
- } => Ok(Expr::Cast(Cast::new(
- Box::new(self.sql_expr_to_logical_expr(
- *expr,
- schema,
- planner_context,
- )?),
- self.convert_data_type(&data_type)?,
- ))),
+ } => {
+ let dt = self.convert_data_type(&data_type)?;
+ let expr =
+ self.sql_expr_to_logical_expr(*expr, schema,
planner_context)?;
+
+ // int/floats input should be treated as seconds rather as
nanoseconds
Review Comment:
```suggestion
// numeric constants are treated as seconds (rather as
nanoseconds)
// to align with postgres / duckdb semantics.
```
##########
datafusion/sql/src/expr/mod.rs:
##########
@@ -224,14 +225,26 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
SQLExpr::Cast {
expr, data_type, ..
- } => Ok(Expr::Cast(Cast::new(
- Box::new(self.sql_expr_to_logical_expr(
- *expr,
- schema,
- planner_context,
- )?),
- self.convert_data_type(&data_type)?,
- ))),
+ } => {
+ let dt = self.convert_data_type(&data_type)?;
+ let expr =
+ self.sql_expr_to_logical_expr(*expr, schema,
planner_context)?;
+
+ // int/floats input should be treated as seconds rather as
nanoseconds
Review Comment:
I was worried about this change as it handles the case of `CAST (x to
TIMESTAMP)` but I was worried that it would be inconsistent with automatic
coercion rules for numbers.
Then I tried some examples and I think there are no automatic coercion rules
for integers to timestamps, which makes sense:
```
❯ select now() - 100000;
Error during planning: Cannot coerce arithmetic expression
Timestamp(Nanosecond, Some("+00:00")) - Int64 to valid types
❯ select now() > 100000;
Error during planning: Cannot infer common argument type for comparison
operation Timestamp(Nanosecond, Some("+00:00")) > Int64
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
