alamb commented on code in PR #8193:
URL: https://github.com/apache/arrow-datafusion/pull/8193#discussion_r1401103621
##########
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:
This comment says "int / floats" but he code only handles Int
I still find this extremely confusing that normally `TIMESTAMP` will map to
`TimestampNanoseconds` but only when used like `122::TIMESTAMP` will it result
in `Timestamp::Second`s
I feel like keeping consistency is more important and adding this special
case will make DataFusion harder to use and understand
##########
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:
are these tests actually different? They look the same to me except the
formatting changed
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -761,13 +761,10 @@ impl BuiltinScalarFunction {
return plan_err!("The to_hex function can only accept
integers.");
}
}),
- BuiltinScalarFunction::ToTimestamp => Ok(match
&input_expr_types[0] {
- Int64 => Timestamp(Second, None),
- _ => Timestamp(Nanosecond, None),
- }),
+ BuiltinScalarFunction::ToTimestamp
+ | BuiltinScalarFunction::ToTimestampNanos =>
Ok(Timestamp(Nanosecond, None)),
Review Comment:
This now switches back to the previous behavior prior to
https://github.com/apache/arrow-datafusion/pull/7844, is that intended?
If so, would it be possible to explain why (and document it clearly so we
don't have to change it again)?
--
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]
