feat: add lit_timestamp_nanosecond #1030
Conversation
datafusion/src/logical_plan/mod.rs
Outdated
| unnormalize_col, unnormalize_cols, upper, when, Column, Expr, ExprRewriter, | ||
| ExpressionVisitor, Literal, Recursion, RewriteRecursion, | ||
| exprlist_to_fields, floor, in_list, initcap, left, length, lit, | ||
| lit_timestamp_nanosecond, ln, log10, log2, lower, lpad, ltrim, max, md5, min, |
There was a problem hiding this comment.
lit_timestamp_nanosecond is newly added
| assert!(maybe_expr.is_err()); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Here are a few examples: is time column has timestamp data type, value 10 needs to also be a timestamp (in nanosecond in this case) to have the expression analysis works
|
@alamb Can you help review this? |
datafusion/src/logical_plan/expr.rs
Outdated
| } | ||
|
|
||
| /// Create a literal timestamp expression | ||
| pub fn lit_timestamp_nanosecond<T: Literal>(n: T) -> Expr { |
There was a problem hiding this comment.
do we consider it a user error if a non-numerical literal type is passed in here? If so, it might be better to introduce a new ns timestamp trait and only implement it for the numeric types we support so we can let rust compiler catch these issues at build time.
There was a problem hiding this comment.
I think it would be nice to try and parse str into timestamps, though that would require making this function fallible, which might be kind of messy. 🤔
There was a problem hiding this comment.
I think a new trait for appropriate numeric only is a good solution. It can be extended later to support string parsing. Let me give it a shot.
datafusion/src/logical_plan/expr.rs
Outdated
|
|
||
| fn lit_timestamp_nanosecond(&self) -> Expr { | ||
| let scalar = ScalarValue::$SCALAR(Some(self.clone())); | ||
| scalar.lit_timestamp_nanosecond() |
There was a problem hiding this comment.
I think this seems a little bit inefficient to wrap a numeric value with ScalarValue enum, then subsequently match on that enum to unpack the value and return a different enum. Perhaps we could find a way to execute ScalarValue::TimestampNanosecond(Some(self.clone().into()) here directly?
There was a problem hiding this comment.
This seems a good solution to me👍
There was a problem hiding this comment.
I have tried that but not all data type works for the into
datafusion/src/logical_plan/expr.rs
Outdated
|
|
||
| fn lit_timestamp_nanosecond(&self) -> Expr { | ||
| let scalar = ScalarValue::$SCALAR(Some(self.clone())); | ||
| scalar.lit_timestamp_nanosecond() |
datafusion/src/logical_plan/expr.rs
Outdated
| } | ||
|
|
||
| /// Create a literal timestamp expression | ||
| pub fn lit_timestamp_nanosecond<T: Literal>(n: T) -> Expr { |
There was a problem hiding this comment.
I think it would be nice to try and parse str into timestamps, though that would require making this function fallible, which might be kind of messy. 🤔
datafusion/src/logical_plan/expr.rs
Outdated
| fn lit(&self) -> Expr; | ||
|
|
||
| /// convert the number to literal expression of timestamp in nanosecond | ||
| fn lit_timestamp_nanosecond(&self) -> Expr; |
There was a problem hiding this comment.
I was thinking perhaps calling this lit_timestamp_nano or something would be more consistent with chrono, but when I looked, chrono appears to be inconsistent on this point (uses both nano and nanosecond) https://docs.rs/chrono/0.4.19/chrono/?search=nano
There was a problem hiding this comment.
I like lit_timestamp_nano better because it is shorter. Let go with it
There was a problem hiding this comment.
@alamb @Dandandan and @houqp I have added a new trait for it. Can you have a look?
|
BTW, we have a python test that fails intermittently, I manually restarted the test. |
4 participants
Which issue does this PR close?
Closes #.
Rationale for this change
Add function
lit_timestamp_nanosecondto create a literal timestamp expression for a number. See tests for examples