You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/07 07:10:30 UTC
[GitHub] [arrow-datafusion] msathis opened a new pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
msathis opened a new pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288
# Which issue does this PR close?
Closes #251 .
# Rationale for this change
Adding Postgres compatible NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
# What changes are included in this PR?
- [ ] Add NOW
- [ ] Multiple NOW should return same time
- [ ] `SELECT NOW(), NOW();` should execute and return timestamps. This doesnt work because both end up with same column name `now`
- [ ] Add CURRENT_TIMESTAMP support
- [ ] Add CURRENT_TIME support
# Are there any user-facing changes?
N/A
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835847872
Thanks @Dandandan That makes sense. I'll modify the PR as per your suggestion 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629883952
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
I think you might be able to pass it via `ScalarFunctionExpr` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629276518
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
I think this needs access too to the `ExecutionProps` so it can use the query start timestamp here.
We can add a test that disables the optimizer rule(s) to show this still resolves to the same value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629181154
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,166 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {}
+
+impl TimestampEvaluation {
+ #[allow(missing_docs)]
+ pub fn new() -> Self {
+ Self {}
+ }
+
+ /// Recursive function to optimize the now expression
+ pub fn optimize_now(&self, exp: &Expr, date_time: &DateTime<Utc>) -> Expr {
+ match exp {
Review comment:
Done. I hope i did it correctly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630072536
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
@Dandandan Will try that today :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835710437
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#288](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1987ac3) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/5f6024d570f4d2d5fd4c2187ff7dcb0cd389ac4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5f6024d) will **decrease** coverage by `0.61%`.
> The diff coverage is `96.48%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 76.80% 76.19% -0.62%
==========================================
Files 133 142 +9
Lines 23284 23927 +643
==========================================
+ Hits 17884 18231 +347
- Misses 5400 5696 +296
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wbGFubmVyLnJz) | `80.00% <80.00%> (-0.37%)` | :arrow_down: |
| [datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2NvbnN0YW50X2ZvbGRpbmcucnM=) | `91.63% <90.74%> (-0.32%)` | :arrow_down: |
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.49% <97.36%> (-0.14%)` | :arrow_down: |
| [...ta/rust/core/src/serde/physical\_plan/from\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9waHlzaWNhbF9wbGFuL2Zyb21fcHJvdG8ucnM=) | `47.80% <100.00%> (+0.25%)` | :arrow_up: |
| [datafusion/src/optimizer/eliminate\_limit.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2VsaW1pbmF0ZV9saW1pdC5ycw==) | `89.18% <100.00%> (+0.30%)` | :arrow_up: |
| [datafusion/src/optimizer/filter\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2ZpbHRlcl9wdXNoX2Rvd24ucnM=) | `97.74% <100.00%> (+<0.01%)` | :arrow_up: |
| [datafusion/src/optimizer/hash\_build\_probe\_order.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2hhc2hfYnVpbGRfcHJvYmVfb3JkZXIucnM=) | `62.06% <100.00%> (-0.54%)` | :arrow_down: |
| [datafusion/src/optimizer/limit\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2xpbWl0X3B1c2hfZG93bi5ycw==) | `97.84% <100.00%> (+0.02%)` | :arrow_up: |
| [datafusion/src/optimizer/projection\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3Byb2plY3Rpb25fcHVzaF9kb3duLnJz) | `98.74% <100.00%> (+0.07%)` | :arrow_up: |
| [datafusion/src/optimizer/utils.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3V0aWxzLnJz) | `49.80% <100.00%> (+0.39%)` | :arrow_up: |
| ... and [60 more](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5f6024d...1987ac3](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628851004
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,160 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {
+ timestamp: DateTime<Utc>,
Review comment:
I think this is a clever approach, indeed the execution context with optimization can be reused across queries.
Also, one could disable the optimization rule (or execute an optimixedy plan). The result of the query needs to be correct in those cases too.
My suggestion would be to add a field to the `ExecutionContext` at the start of a query pass it to the optimizers / evaluation code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-841159026
Yes, amazing work @msathis
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jorgecarleitao commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-839705580
AFAIK `current_*` are all derived from `now`; imo the differentiator aspect here is that there is some state `X` that is being shared.
It seems to me that the use-case here is that we want to preserve state across nodes, so that their execution depends on said state. `NOW` is an example, but in reality, `random` is also an example; we "cheated" a bit by not allowing users to select a seed. If they want that, we hit the same problem as NOW.
IMO a natural construct here is something like `struct StatefulFunction<T: Send + Sync>`, where `T` is the state, and `Arc<T>` is inside of it, and that implements `PhysicalExpr`. During planning, the initial state is passed to it from the planner, and we are ready to fly.
For Ballista, we would probably need `T` to also be "proto-able", but we can handle that on a future PR when we add support for this node to Ballista.
IMO this would enable us to implement `now`, but also a lot of other functions that share state (e.g. pass `T: Mutex<something>` and we have a function that can change state during evaluation, which AFAIK is what we need for window functions, whereby the state is updated as batches pass through it).
The `ScalarFunction` construct was meant to be stateless because it makes it very easy to develop, and it also makes it obvious that is stateless. Trying to couple execution state to them is imo going beyond its scope.
IMO a `PhysicalExpr` is not really that difficult to implement; it is just a `struct` with `evaluate`, `schema`, and a new match on the physical planner.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628851439
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,166 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {}
+
+impl TimestampEvaluation {
+ #[allow(missing_docs)]
+ pub fn new() -> Self {
+ Self {}
+ }
+
+ /// Recursive function to optimize the now expression
+ pub fn optimize_now(&self, exp: &Expr, date_time: &DateTime<Utc>) -> Expr {
+ match exp {
Review comment:
Did i miss any other possible optimizations?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628850571
##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -3611,17 +3607,19 @@ mod tests {
Ok(())
}
- #[test]
- fn test_concat_error() -> Result<()> {
Review comment:
This error is now thrown during execution instead of logical plan. I can fix the test case to check execution error if required.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630036998
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,177 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::execution::context::ExecutionProps;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
Review comment:
Done. Moved the logic to `constant_folding`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630071639
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -200,6 +209,15 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
Expr::Not(inner)
}
}
+ Expr::ScalarFunction {
Review comment:
@alamb I have to solve the non-optimized plan case yet (PR description has subtasks). Can you please add `Do not merge` label till then?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630418124
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
Might also good to document this at the constant folding optimizer if we don't use the timestamp here - that the pass is required for generating correct queries.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r632372342
##########
File path: datafusion/src/execution/context.rs
##########
@@ -740,6 +748,15 @@ impl ExecutionConfig {
}
}
+/// Holds per-execution properties and data (such as starting timestamps, etc).
+/// An instance of this struct is created each time a [`LogicalPlan`] is prepared for
+/// execution (optimized). If the same plan is optimized multiple times, a new
+/// `ExecutionProps` is created each time.
+#[derive(Clone)]
+pub struct ExecutionProps {
+ pub(crate) query_execution_start_time: Option<DateTime<Utc>>,
Review comment:
Could we make this `DateTime<Utc>` per the discussion?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629262731
##########
File path: datafusion/src/execution/context.rs
##########
@@ -740,6 +748,12 @@ impl ExecutionConfig {
}
}
+/// Current execution props
Review comment:
```suggestion
/// Holds per-execution properties and data (such as starting timestamps, etc).
/// An instance of this struct is created each time a [`LogicalPlan`] is prepared for
/// execution (optimized). If the same plan is optimized multiple times, a new
/// `ExecutionProps` is created each time.
```
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,177 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::execution::context::ExecutionProps;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
Review comment:
What would you think about extending the `constant_folding` optimizer rule instead of adding an entirely new rule?
I think you could probably add a case to the expression rewriter in https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/optimizer/constant_folding.rs#L122 for `Expr::ScalarFunction { fun: BuiltinScalarFunction::Now}}` and avoid the need for all the code in this pass for traversing the expression tree.
##########
File path: datafusion/tests/sql.rs
##########
@@ -2738,6 +2738,24 @@ async fn test_cast_expressions() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn test_timestamp_expressions() -> Result<()> {
+ let t1 = chrono::Utc::now().timestamp();
+ let mut ctx = ExecutionContext::new();
+ let actual = execute(&mut ctx, "SELECT NOW(), NOW() as t2").await;
+ let res1 = actual[0][0].as_str();
+ let res2 = actual[0][1].as_str();
+ let t3 = chrono::Utc::now().timestamp();
+ let t2_naive =
Review comment:
👍 good test.
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,177 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::execution::context::ExecutionProps;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {}
+
+impl TimestampEvaluation {
+ #[allow(missing_docs)]
+ pub fn new() -> Self {
+ Self {}
+ }
+
+ /// Recursive function to optimize the now expression
+ pub fn rewrite_expr(&self, exp: &Expr, date_time: &DateTime<Utc>) -> Result<Expr> {
+ let expressions = utils::expr_sub_expressions(exp).unwrap();
+ let expressions = expressions
+ .iter()
+ .map(|e| self.rewrite_expr(e, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ let exp = match exp {
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Now,
+ ..
+ } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+ date_time.timestamp_nanos(),
+ ))),
+ _ => exp.clone(),
+ };
+ utils::rewrite_expression(&exp, &expressions)
+ }
+
+ fn optimize_with_datetime(
+ &self,
+ plan: &LogicalPlan,
+ date_time: &DateTime<Utc>,
+ ) -> Result<LogicalPlan> {
+ match plan {
+ LogicalPlan::Projection { .. } => {
+ let exprs = plan
+ .expressions()
+ .iter()
+ .map(|exp| self.rewrite_expr(exp, date_time).unwrap())
+ .collect::<Vec<_>>();
+
+ // apply the optimization to all inputs of the plan
+ let inputs = plan.inputs();
+ let new_inputs = inputs
+ .iter()
+ .map(|plan| self.optimize_with_datetime(*plan, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ utils::from_plan(plan, &exprs, &new_inputs)
+ }
+ _ => {
+ let expr = plan.expressions();
+
+ // apply the optimization to all inputs of the plan
+ let inputs = plan.inputs();
+ let new_inputs = inputs
+ .iter()
+ .map(|plan| self.optimize_with_datetime(*plan, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ utils::from_plan(plan, &expr, &new_inputs)
+ }
+ }
+ }
+}
+
+impl OptimizerRule for TimestampEvaluation {
+ fn optimize(
+ &self,
+ plan: &LogicalPlan,
+ props: &ExecutionProps,
+ ) -> Result<LogicalPlan> {
+ self.optimize_with_datetime(plan, &props.query_execution_start_time.unwrap())
+ }
+
+ fn name(&self) -> &str {
+ "timestamp_evaluation"
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::logical_plan::LogicalPlanBuilder;
+ use crate::test::*;
+
+ fn get_optimized_plan_formatted(plan: &LogicalPlan) -> String {
+ let rule = TimestampEvaluation::new();
+ let execution_props = ExecutionProps {
+ query_execution_start_time: Some(chrono::Utc::now()),
+ };
+
+ let optimized_plan = rule
+ .optimize(plan, &execution_props)
+ .expect("failed to optimize plan");
+ return format!("{:?}", optimized_plan);
+ }
+
+ #[test]
+ fn single_now() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = "Projection: TimestampNanosecond(";
+ assert!(get_optimized_plan_formatted(&plan).starts_with(expected));
+ }
+
+ #[test]
+ fn double_now() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![
+ Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ },
+ Expr::Alias(
+ Box::new(Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ }),
+ "t2".to_string(),
+ ),
+ ];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let actual = get_optimized_plan_formatted(&plan);
+ println!("output is {}", &actual);
+ let expected_start = "Projection: TimestampNanosecond(";
+ assert!(actual.starts_with(expected_start));
+
+ let expected_end = ") AS t2\
+ \n TableScan: test projection=None";
+ assert!(actual.ends_with(expected_end));
Review comment:
It would probably be good here to ensure the same timestamp value was produced in place `now()` was replaced.
Since you can specify what timestamp value goes in perhaps you could hard code it in the test (so you could compare the plan with a known constant string). Perhaps something like:
```
let start_time = Utc.ymd(2018, 7, 1).and_hms(6, 0, 0); // 2018-Jul-01 06:00
let execution_props = ExecutionProps {
query_execution_start_time: Some(start_time),
};
```
##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -3611,17 +3607,19 @@ mod tests {
Ok(())
}
- #[test]
- fn test_concat_error() -> Result<()> {
Review comment:
I think it would be valuable to update the test case
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840491188
I am not opposed to the idea of `StatefulFunction` but it seems like a large overkill for the now() function.
Here is an alternate proposal (in https://github.com/apache/arrow-datafusion/pull/335): https://github.com/apache/arrow-datafusion/commit/261e76982178dfcc723aa9b2842e7b4d1d4a4b87 -- basically use a closure to capture the value for now() during plan time. It doesn't require changing any other function signatures and I think implements the semantics of `now()` correctly.
What do you think @msathis @jorgecarleitao @Dandandan and @returnString ?
If we still want to do `StatefulFunction` that is cool too, but perhaps it would better be done as part of another PR?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629301375
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
Thanks @Dandandan . I just started working on this. But finding it quite difficult to pass `ExecutionProps` to these functions. There are multiple types of `ExecutionPlan`. Do i need to add a reference of `ExecutionProps` to all of these?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835811345
@msathis
Thanks for this PR!
I believe we should do it a bit differently and not calculate the timestamp inside the optimization rule.
One could disable the optimization rule, or execute an un-optimized plan.
The result of the query needs to be correct in those cases too.
My suggestion would be to set field to the `ExecutionContextState` (something like `queryExecutionStartTime`) at the start of a query and pass it to the optimizers / evaluation code. This requires maybe some more changes to the code, but is needed to correctly run the queries.
For the optimizer you could use `utils::expr_sub_expressions` (see use in filter push down) to recurse into the expressions and replace *all* cases of the `NOW` with the timestamp of the start of the query.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r632307469
##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -3611,17 +3607,19 @@ mod tests {
Ok(())
}
- #[test]
- fn test_concat_error() -> Result<()> {
Review comment:
Done 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
returnString commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840494765
Yeah on the basis that `now` is quite a common/useful function and this PR plus @alamb's closure change resolves this in a neat way, my suggestion would be:
- try to merge this (excellent!) work with #335 on top to address the concerns around high-impact signature changes
- plan future work to better support different "classes" of function at both the logical layer and the physical (as @jorgecarleitao explained above)
Especially at the logical level, I think there are _more_ potential benefits here than just supporting stateful functions outright, like enabling better plan optimisation, and we could probably do with some upfront design work on that first 😄
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628850408
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,160 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {
+ timestamp: DateTime<Utc>,
Review comment:
Now timestamp is initialised during the actual optimize call
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
returnString commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-839833123
@jorgecarleitao that all sounds reasonable! In Postgres, this sort of corresponds to the function volatility categories (https://www.postgresql.org/docs/13/xfunc-volatility.html) which might be a useful basis for any future definition of different function types.
- immutable: pure function, can only use arguments and internal constants (example: basic math ops). Optimiser can do lots here
- stable: can refer to shared state but must return the same value for the same arguments within a given statement (example: `now`). Optimiser is allowed to unify all references into one call per unique set of arguments
- volatile: no rules, no optimiser potential! Must always be evaluated exactly as initially planned (example: `random`)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-841148743
Thanks @jorgecarleitao! It was a great learning experience for me. 👌
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
returnString commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840800353
> I also am not sure I would call now() "stateful" in the sense that it has state that changes during the execution of the query. It is more like "parameterized" or something with a parameter that is filled in prior to execution
Yeah, in Postgres parlance this kind of function is "stable" (read: consistent over the course of a single txn given the same inputs) as opposed to "immutable" (which can't reference _anything_ outside its args/constants) - still not the best breakdown imo, but a little bit closer maybe.
Cache invalidation and naming things, right? 😅
> Also, I am not convinced about how valuable a general purpose StatefulFunction will be (though of course it depends on a proposal that is not yet written ;) )
Off the top of my head I think it'll open up some potential for generalised optimisation passes over function usage in queries according to function class, i.e. the optimiser rule used for the initial implementation of this PR but applicable to arbitrary functions provided they indicate themselves to be "stable".
> is it fair to say that there is some urgency in the NOW? In this case, I agree that the closure is a great stop-gap and we should go for it.
Personally I think so, it's a pretty generally useful function and opens up lots of good time-series use cases as @alamb mentioned.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629289040
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,177 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::execution::context::ExecutionProps;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {}
+
+impl TimestampEvaluation {
+ #[allow(missing_docs)]
+ pub fn new() -> Self {
+ Self {}
+ }
+
+ /// Recursive function to optimize the now expression
+ pub fn rewrite_expr(&self, exp: &Expr, date_time: &DateTime<Utc>) -> Result<Expr> {
+ let expressions = utils::expr_sub_expressions(exp).unwrap();
+ let expressions = expressions
+ .iter()
+ .map(|e| self.rewrite_expr(e, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ let exp = match exp {
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Now,
+ ..
+ } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+ date_time.timestamp_nanos(),
+ ))),
+ _ => exp.clone(),
+ };
+ utils::rewrite_expression(&exp, &expressions)
+ }
+
+ fn optimize_with_datetime(
+ &self,
+ plan: &LogicalPlan,
+ date_time: &DateTime<Utc>,
+ ) -> Result<LogicalPlan> {
+ match plan {
+ LogicalPlan::Projection { .. } => {
+ let exprs = plan
+ .expressions()
+ .iter()
+ .map(|exp| self.rewrite_expr(exp, date_time).unwrap())
+ .collect::<Vec<_>>();
+
+ // apply the optimization to all inputs of the plan
+ let inputs = plan.inputs();
+ let new_inputs = inputs
+ .iter()
+ .map(|plan| self.optimize_with_datetime(*plan, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ utils::from_plan(plan, &exprs, &new_inputs)
+ }
+ _ => {
+ let expr = plan.expressions();
+
+ // apply the optimization to all inputs of the plan
+ let inputs = plan.inputs();
+ let new_inputs = inputs
+ .iter()
+ .map(|plan| self.optimize_with_datetime(*plan, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ utils::from_plan(plan, &expr, &new_inputs)
+ }
+ }
+ }
+}
+
+impl OptimizerRule for TimestampEvaluation {
+ fn optimize(
+ &self,
+ plan: &LogicalPlan,
+ props: &ExecutionProps,
+ ) -> Result<LogicalPlan> {
+ self.optimize_with_datetime(plan, &props.query_execution_start_time.unwrap())
+ }
+
+ fn name(&self) -> &str {
+ "timestamp_evaluation"
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::logical_plan::LogicalPlanBuilder;
+ use crate::test::*;
+
+ fn get_optimized_plan_formatted(plan: &LogicalPlan) -> String {
+ let rule = TimestampEvaluation::new();
+ let execution_props = ExecutionProps {
+ query_execution_start_time: Some(chrono::Utc::now()),
+ };
+
+ let optimized_plan = rule
+ .optimize(plan, &execution_props)
+ .expect("failed to optimize plan");
+ return format!("{:?}", optimized_plan);
+ }
+
+ #[test]
+ fn single_now() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = "Projection: TimestampNanosecond(";
+ assert!(get_optimized_plan_formatted(&plan).starts_with(expected));
+ }
+
+ #[test]
+ fn double_now() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![
+ Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ },
+ Expr::Alias(
+ Box::new(Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ }),
+ "t2".to_string(),
+ ),
+ ];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let actual = get_optimized_plan_formatted(&plan);
+ println!("output is {}", &actual);
+ let expected_start = "Projection: TimestampNanosecond(";
+ assert!(actual.starts_with(expected_start));
+
+ let expected_end = ") AS t2\
+ \n TableScan: test projection=None";
+ assert!(actual.ends_with(expected_end));
Review comment:
Great suggestion. How did i miss this? 🤕
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630072536
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
Will try that today :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840763111
@jorgecarleitao Also, I am not convinced about how valuable a general purpose `StatefulFunction` will be (though of course it depends on a proposal that is not yet written ;) )
This is due to:
2. You can write such a function today as a `ScalarFunction` -- via a closure with an `Arc<...>` with shared state. I don't see much advantage to building that into DataFusion. Window functions I think are going to have to be their own thing (like `SortExpr` and `AggregateExpr`.
1. Given there is no way to know what order DataFusion will pass inputs to your functions (across threads, repartitioned, etc) I can't think of a usecase at this moment 🤔
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630037254
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,177 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::execution::context::ExecutionProps;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {}
+
+impl TimestampEvaluation {
+ #[allow(missing_docs)]
+ pub fn new() -> Self {
+ Self {}
+ }
+
+ /// Recursive function to optimize the now expression
+ pub fn rewrite_expr(&self, exp: &Expr, date_time: &DateTime<Utc>) -> Result<Expr> {
+ let expressions = utils::expr_sub_expressions(exp).unwrap();
+ let expressions = expressions
+ .iter()
+ .map(|e| self.rewrite_expr(e, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ let exp = match exp {
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Now,
+ ..
+ } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+ date_time.timestamp_nanos(),
+ ))),
+ _ => exp.clone(),
+ };
+ utils::rewrite_expression(&exp, &expressions)
+ }
+
+ fn optimize_with_datetime(
+ &self,
+ plan: &LogicalPlan,
+ date_time: &DateTime<Utc>,
+ ) -> Result<LogicalPlan> {
+ match plan {
+ LogicalPlan::Projection { .. } => {
+ let exprs = plan
+ .expressions()
+ .iter()
+ .map(|exp| self.rewrite_expr(exp, date_time).unwrap())
+ .collect::<Vec<_>>();
+
+ // apply the optimization to all inputs of the plan
+ let inputs = plan.inputs();
+ let new_inputs = inputs
+ .iter()
+ .map(|plan| self.optimize_with_datetime(*plan, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ utils::from_plan(plan, &exprs, &new_inputs)
+ }
+ _ => {
+ let expr = plan.expressions();
+
+ // apply the optimization to all inputs of the plan
+ let inputs = plan.inputs();
+ let new_inputs = inputs
+ .iter()
+ .map(|plan| self.optimize_with_datetime(*plan, date_time))
+ .collect::<Result<Vec<_>>>()?;
+
+ utils::from_plan(plan, &expr, &new_inputs)
+ }
+ }
+ }
+}
+
+impl OptimizerRule for TimestampEvaluation {
+ fn optimize(
+ &self,
+ plan: &LogicalPlan,
+ props: &ExecutionProps,
+ ) -> Result<LogicalPlan> {
+ self.optimize_with_datetime(plan, &props.query_execution_start_time.unwrap())
+ }
+
+ fn name(&self) -> &str {
+ "timestamp_evaluation"
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use crate::logical_plan::LogicalPlanBuilder;
+ use crate::test::*;
+
+ fn get_optimized_plan_formatted(plan: &LogicalPlan) -> String {
+ let rule = TimestampEvaluation::new();
+ let execution_props = ExecutionProps {
+ query_execution_start_time: Some(chrono::Utc::now()),
+ };
+
+ let optimized_plan = rule
+ .optimize(plan, &execution_props)
+ .expect("failed to optimize plan");
+ return format!("{:?}", optimized_plan);
+ }
+
+ #[test]
+ fn single_now() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ }];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let expected = "Projection: TimestampNanosecond(";
+ assert!(get_optimized_plan_formatted(&plan).starts_with(expected));
+ }
+
+ #[test]
+ fn double_now() {
+ let table_scan = test_table_scan().unwrap();
+ let proj = vec![
+ Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ },
+ Expr::Alias(
+ Box::new(Expr::ScalarFunction {
+ args: vec![],
+ fun: BuiltinScalarFunction::Now,
+ }),
+ "t2".to_string(),
+ ),
+ ];
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .project(proj)
+ .unwrap()
+ .build()
+ .unwrap();
+
+ let actual = get_optimized_plan_formatted(&plan);
+ println!("output is {}", &actual);
+ let expected_start = "Projection: TimestampNanosecond(";
+ assert!(actual.starts_with(expected_start));
+
+ let expected_end = ") AS t2\
+ \n TableScan: test projection=None";
+ assert!(actual.ends_with(expected_end));
Review comment:
Done ✌️
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jorgecarleitao commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840584438
Just to understand, @alamb and @returnString , is it fair to say that there is some urgency in the NOW? In this case, I agree that the closure is a great stop-gap and we should go for it.
Other than that, if you want to take a stab at stateful functions, @msathis , an idea is to create an issue and a design document / draft, so that others could comment on before you commit to a large chunk of work?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835710437
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#288](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (930aaae) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/5f6024d570f4d2d5fd4c2187ff7dcb0cd389ac4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5f6024d) will **decrease** coverage by `0.62%`.
> The diff coverage is `95.91%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 76.80% 76.18% -0.63%
==========================================
Files 133 142 +9
Lines 23284 23917 +633
==========================================
+ Hits 17884 18220 +336
- Misses 5400 5697 +297
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/physical\_plan/type\_coercion.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi90eXBlX2NvZXJjaW9uLnJz) | `98.65% <75.00%> (-0.66%)` | :arrow_down: |
| [datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9wbGFubmVyLnJz) | `80.00% <80.00%> (-0.37%)` | :arrow_down: |
| [datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2NvbnN0YW50X2ZvbGRpbmcucnM=) | `91.63% <90.74%> (-0.32%)` | :arrow_down: |
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.49% <97.36%> (-0.14%)` | :arrow_down: |
| [...ta/rust/core/src/serde/physical\_plan/from\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9waHlzaWNhbF9wbGFuL2Zyb21fcHJvdG8ucnM=) | `47.80% <100.00%> (+0.25%)` | :arrow_up: |
| [datafusion/src/optimizer/eliminate\_limit.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2VsaW1pbmF0ZV9saW1pdC5ycw==) | `89.18% <100.00%> (+0.30%)` | :arrow_up: |
| [datafusion/src/optimizer/filter\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2ZpbHRlcl9wdXNoX2Rvd24ucnM=) | `97.74% <100.00%> (+<0.01%)` | :arrow_up: |
| [datafusion/src/optimizer/hash\_build\_probe\_order.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2hhc2hfYnVpbGRfcHJvYmVfb3JkZXIucnM=) | `62.06% <100.00%> (-0.54%)` | :arrow_down: |
| [datafusion/src/optimizer/limit\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2xpbWl0X3B1c2hfZG93bi5ycw==) | `97.84% <100.00%> (+0.02%)` | :arrow_up: |
| [datafusion/src/optimizer/projection\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3Byb2plY3Rpb25fcHVzaF9kb3duLnJz) | `98.74% <100.00%> (+0.07%)` | :arrow_up: |
| ... and [60 more](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5f6024d...930aaae](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840305737
I think we should create another category of functions called `StatefulFunction` as @jorgecarleitao explained above. This seems like a clean solution without any hacks from my point of view. But open for other suggestions as well. I would like to give this issue another try once we have the agreement on which way to go.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630061415
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -200,6 +209,15 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
Expr::Not(inner)
}
}
+ Expr::ScalarFunction {
Review comment:
❤️ looking very nice
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -200,6 +209,15 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
Expr::Not(inner)
}
}
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Now,
+ ..
+ } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+ self.execution_props
+ .query_execution_start_time
+ .unwrap()
Review comment:
It may be worth a comment here about why the code assumes that `query_execution_start_time` is set -- it is always set prior to the optimizer being run.
While I am typing this, I almost wonder if we could change `ExecutionProps` so that `query_execution_time` was always set (set it on construction). We could do that as a follow on PR perhaps
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
It seems to me that this function should never actually be called if the plan has been optimized (as this function call should have been replaced by a scalar function during constant folding).
Thus if this code is hit, wouldn't it be a bug? I can think of two possible behaviors:
1. The code we have now (generate a value, but not the intended one)
2. Return an error.
If we return an error it starts meaning we can't run plans without the optimizer which may not be ideal (I am not sure).
Thus, in terms of "being nice to users" I would suggest option 1 and add a `warn!` here saying that the plan hasn't been optimized.
```suggestion
// this function should have been replaced with a constant as part of optimization
// it this code is being run it likely means 1) there is a bug in constant folding or
// 2) the plan was not optimized prior to being run.
warn!("now() function has not been optimized");
Some(chrono::Utc::now().timestamp_nanos()),
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835710437
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#288](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9335a70) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/5f6024d570f4d2d5fd4c2187ff7dcb0cd389ac4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5f6024d) will **decrease** coverage by `0.40%`.
> The diff coverage is `94.85%`.
> :exclamation: Current head 9335a70 differs from pull request most recent head 12a4964. Consider uploading reports for the commit 12a4964 to get more accurate results
[](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 76.80% 76.40% -0.41%
==========================================
Files 133 142 +9
Lines 23284 23856 +572
==========================================
+ Hits 17884 18227 +343
- Misses 5400 5629 +229
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...tafusion/src/physical\_plan/datetime\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kYXRldGltZV9leHByZXNzaW9ucy5ycw==) | `67.51% <0.00%> (-1.32%)` | :arrow_down: |
| [datafusion/src/physical\_plan/type\_coercion.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi90eXBlX2NvZXJjaW9uLnJz) | `98.65% <75.00%> (-0.66%)` | :arrow_down: |
| [datafusion/src/optimizer/timestamp\_evaluation.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3RpbWVzdGFtcF9ldmFsdWF0aW9uLnJz) | `93.65% <93.65%> (ø)` | |
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.65% <97.29%> (+0.01%)` | :arrow_up: |
| [...ta/rust/core/src/serde/physical\_plan/from\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9waHlzaWNhbF9wbGFuL2Zyb21fcHJvdG8ucnM=) | `47.80% <100.00%> (+0.25%)` | :arrow_up: |
| [datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2NvbnN0YW50X2ZvbGRpbmcucnM=) | `91.98% <100.00%> (+0.03%)` | :arrow_up: |
| [datafusion/src/optimizer/eliminate\_limit.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2VsaW1pbmF0ZV9saW1pdC5ycw==) | `89.18% <100.00%> (+0.30%)` | :arrow_up: |
| [datafusion/src/optimizer/filter\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2ZpbHRlcl9wdXNoX2Rvd24ucnM=) | `97.74% <100.00%> (+<0.01%)` | :arrow_up: |
| [datafusion/src/optimizer/hash\_build\_probe\_order.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2hhc2hfYnVpbGRfcHJvYmVfb3JkZXIucnM=) | `62.06% <100.00%> (-0.54%)` | :arrow_down: |
| [datafusion/src/optimizer/limit\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2xpbWl0X3B1c2hfZG93bi5ycw==) | `97.84% <100.00%> (+0.02%)` | :arrow_up: |
| ... and [41 more](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5f6024d...12a4964](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630073584
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -200,6 +209,15 @@ impl<'a> ExprRewriter for ConstantRewriter<'a> {
Expr::Not(inner)
}
}
+ Expr::ScalarFunction {
+ fun: BuiltinScalarFunction::Now,
+ ..
+ } => Expr::Literal(ScalarValue::TimestampNanosecond(Some(
+ self.execution_props
+ .query_execution_start_time
+ .unwrap()
Review comment:
I can make it `DateTime<Utc>` instead of `Option<DateTime<Utc>>`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840759475
@jorgecarleitao I would like `now()` to be added to datafusion in the next few weeks -- my usecase is that I want to be able to write queries with predicates like "in the last 5 minutes" (which are very common in timeseries queries). The standard sql I know to do this is
```
WHERE ts > now() - interval '5 minutes'
```
However, that query also requires timestamp arithmetic -- part of https://github.com/apache/arrow-datafusion/issues/194 -- but I can work around the lack of https://github.com/apache/arrow-datafusion/issues/194 with casting. I can't think of any way to work around the lack of `now()`
I also am not sure I would call `now()` "stateful" in the sense that it has state that changes during the execution of the query. It is more like "parameterized" or something with a parameter that is filled in prior to execution
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
returnString edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840494765
Yeah on the basis that `now` is quite a common/useful function and this PR plus @alamb's closure change resolves this in a neat way, my suggestion would be:
- try to merge this (excellent!) work with #335 on top to address the concerns around high-impact signature changes
- plan future work to better support different "classes" of function at both the logical layer and the physical (as @jorgecarleitao explained above)
Especially at the logical level, I think there are _more_ potential benefits here than just supporting stateful functions outright, like enabling better plan optimisation, and we could probably do with some upfront design work on that first 😄
Edit: also, if/when we land some work for stateful functions, there's nothing that stops us migrating this across later!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-841036217
I have reverted the last commit & added @alamb approach from #335. Took care of the review comments as well. @alamb @jorgecarleitao Can you please give another look at the PR?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-841408299
I filed https://github.com/apache/arrow-datafusion/issues/340 to track possible future work on stateful functions
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629301758
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,177 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::execution::context::ExecutionProps;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
Review comment:
Makes sense. Will do this. Will simplify the code a lot.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r629276518
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
I think this needs access too to the `ExecutionProps` so it can use the query start timestamp here.
We can add a test that disables the optimizer rule(s) to show this still resolves to the same value as in `ExecutionProps` and has the same value for `SELECT NOW() one, NOW() two`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter commented on pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835710437
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#288](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (25d50f8) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/5f6024d570f4d2d5fd4c2187ff7dcb0cd389ac4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5f6024d) will **decrease** coverage by `0.66%`.
> The diff coverage is `90.54%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 76.80% 76.14% -0.67%
==========================================
Files 133 141 +8
Lines 23284 23706 +422
==========================================
+ Hits 17884 18050 +166
- Misses 5400 5656 +256
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.64% <ø> (+0.01%)` | :arrow_up: |
| [...tafusion/src/physical\_plan/datetime\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kYXRldGltZV9leHByZXNzaW9ucy5ycw==) | `67.51% <0.00%> (-1.32%)` | :arrow_down: |
| [datafusion/src/physical\_plan/type\_coercion.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi90eXBlX2NvZXJjaW9uLnJz) | `98.65% <75.00%> (-0.66%)` | :arrow_down: |
| [datafusion/src/optimizer/timestamp\_evaluation.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3RpbWVzdGFtcF9ldmFsdWF0aW9uLnJz) | `95.08% <95.08%> (ø)` | |
| [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9mdW5jdGlvbnMucnM=) | `89.46% <100.00%> (-0.03%)` | :arrow_down: |
| [datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi90ZXN0cy9zcWwucnM=) | `99.89% <100.00%> (+<0.01%)` | :arrow_up: |
| [datafusion/src/physical\_plan/coalesce\_batches.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9jb2FsZXNjZV9iYXRjaGVzLnJz) | `83.18% <0.00%> (-1.77%)` | :arrow_down: |
| [...tafusion/src/physical\_plan/distinct\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kaXN0aW5jdF9leHByZXNzaW9ucy5ycw==) | `90.34% <0.00%> (-0.57%)` | :arrow_down: |
| [datafusion/src/datasource/csv.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZGF0YXNvdXJjZS9jc3YucnM=) | `73.07% <0.00%> (-0.26%)` | :arrow_down: |
| [...lista/rust/core/src/serde/logical\_plan/to\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9sb2dpY2FsX3BsYW4vdG9fcHJvdG8ucnM=) | `68.05% <0.00%> (-0.10%)` | :arrow_down: |
| ... and [22 more](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5f6024d...25d50f8](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-839993301
The stateful function seems needed at some point.
I am not sure a stateful function would be needed here, at this moment, and might go a bit beyond the scope of this PR/feature(?).
Adding a node for node for timestamps (which can be used by current_date, NOW(), etc.) which only has access to the query start time seems like a more natural route to me for now. WDYT?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r632392077
##########
File path: datafusion/src/execution/context.rs
##########
@@ -740,6 +748,15 @@ impl ExecutionConfig {
}
}
+/// Holds per-execution properties and data (such as starting timestamps, etc).
+/// An instance of this struct is created each time a [`LogicalPlan`] is prepared for
+/// execution (optimized). If the same plan is optimized multiple times, a new
+/// `ExecutionProps` is created each time.
+#[derive(Clone)]
+pub struct ExecutionProps {
+ pub(crate) query_execution_start_time: Option<DateTime<Utc>>,
Review comment:
Done 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835710437
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#288](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (24c5bf5) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/5f6024d570f4d2d5fd4c2187ff7dcb0cd389ac4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5f6024d) will **decrease** coverage by `0.66%`.
> The diff coverage is `89.33%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 76.80% 76.13% -0.67%
==========================================
Files 133 141 +8
Lines 23284 23709 +425
==========================================
+ Hits 17884 18052 +168
- Misses 5400 5657 +257
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.64% <ø> (+0.01%)` | :arrow_up: |
| [...tafusion/src/physical\_plan/datetime\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kYXRldGltZV9leHByZXNzaW9ucy5ycw==) | `67.51% <0.00%> (-1.32%)` | :arrow_down: |
| [datafusion/src/physical\_plan/type\_coercion.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi90eXBlX2NvZXJjaW9uLnJz) | `98.65% <75.00%> (-0.66%)` | :arrow_down: |
| [datafusion/src/optimizer/timestamp\_evaluation.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3RpbWVzdGFtcF9ldmFsdWF0aW9uLnJz) | `93.54% <93.54%> (ø)` | |
| [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9mdW5jdGlvbnMucnM=) | `89.46% <100.00%> (-0.03%)` | :arrow_down: |
| [datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi90ZXN0cy9zcWwucnM=) | `99.89% <100.00%> (+<0.01%)` | :arrow_up: |
| [datafusion/src/physical\_plan/coalesce\_batches.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9jb2FsZXNjZV9iYXRjaGVzLnJz) | `83.18% <0.00%> (-1.77%)` | :arrow_down: |
| [...tafusion/src/physical\_plan/distinct\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kaXN0aW5jdF9leHByZXNzaW9ucy5ycw==) | `90.34% <0.00%> (-0.57%)` | :arrow_down: |
| [datafusion/src/datasource/csv.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZGF0YXNvdXJjZS9jc3YucnM=) | `73.07% <0.00%> (-0.26%)` | :arrow_down: |
| [...lista/rust/core/src/serde/logical\_plan/to\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9sb2dpY2FsX3BsYW4vdG9fcHJvdG8ucnM=) | `68.05% <0.00%> (-0.10%)` | :arrow_down: |
| ... and [22 more](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5f6024d...24c5bf5](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628884241
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,166 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {}
+
+impl TimestampEvaluation {
+ #[allow(missing_docs)]
+ pub fn new() -> Self {
+ Self {}
+ }
+
+ /// Recursive function to optimize the now expression
+ pub fn optimize_now(&self, exp: &Expr, date_time: &DateTime<Utc>) -> Expr {
+ match exp {
Review comment:
You'll want to recurse in any `Expr` so it finds all timestamps, regardless how deep.
E.g. `ts >= NOW()` etc.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840101186
> The stateful function seems needed at some point. I am not sure a stateful function would be needed here, at this moment, and might go a bit beyond the scope of this PR/feature(?).
I agree with @Dandandan -- I would like to get a basic implementation of now(). I feel @msathis had this PR really close but then the addition of `ExecutionProps` to all scalar functions has caused issues
> Adding a node for node for timestamps (which can be used by current_date, NOW(), etc.) which only has access to the query start time seems like a more natural route to me for now. WDYT?
Just to be clear, are you suggesting adding a new Expr variant, something like `Expr::now()` that could be handled specially?
I guess I was hoping that part of the translation from Expr --> PhysicalExpr could somehow capture state that was available at plan time (e.g. `ExecutionProps`) without having to change the signature of all scalar functions. Perhaps we could use a closure or something to capture this statel
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835723978
This PR is already big. To reduce the review burden, Will add CURRENT_TIMESTAMP and CURRENT_TIME in a separate PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840759475
@jorgecarleitao I would like `now()` to be added to datafusion in the next few weeks -- my usecase is that I want to be able to write queries with predicates like "in the last 5 minutes" (which are very common in timeseries queries). The standard sql I know to do this is
```
WHERE ts > now() - interval '5 minutes'
```
However, that query also requires timestamp arithmetic -- part of https://github.com/apache/arrow-datafusion/issues/194 -- but I can work around the lack of https://github.com/apache/arrow-datafusion/issues/194 with casting. I can't think of any way to work around the lack of `now()`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628845454
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,160 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {
+ timestamp: DateTime<Utc>,
Review comment:
@alamb Do you think this approach is correct? I have my doubts as we might create an optimizer and reuse across multiple queries.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r628845454
##########
File path: datafusion/src/optimizer/timestamp_evaluation.rs
##########
@@ -0,0 +1,160 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Optimizer rule to replace timestamp expressions to constants.
+//! This saves time in planning and executing the query.
+use crate::error::Result;
+use crate::logical_plan::{Expr, LogicalPlan};
+use crate::optimizer::optimizer::OptimizerRule;
+
+use super::utils;
+use crate::physical_plan::functions::BuiltinScalarFunction;
+use crate::scalar::ScalarValue;
+use chrono::{DateTime, Utc};
+
+/// Optimization rule that replaces timestamp expressions with their values evaluated
+pub struct TimestampEvaluation {
+ timestamp: DateTime<Utc>,
Review comment:
@alamb Do you think this approach is correct? I have my doubts as we might create an optimizer and reuse it across multiple queries.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-839688275
> I am not sure we should do this: we are changing all functions' signatures because of a single function's requirement.
>
> I think we should add a node specifically for `NOW` and leave all the other functions' alone.
@jorgecarleitao That's a good question. Even i was wondering about that. @alamb @Dandandan your take?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] msathis commented on a change in pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
msathis commented on a change in pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#discussion_r630097221
##########
File path: datafusion/src/physical_plan/datetime_expressions.rs
##########
@@ -268,6 +268,13 @@ pub fn to_timestamp(args: &[ColumnarValue]) -> Result<ColumnarValue> {
)
}
+/// now SQL function
+pub fn now(_: &[ColumnarValue]) -> Result<ColumnarValue> {
+ Ok(ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
+ Some(chrono::Utc::now().timestamp_nanos()),
Review comment:
@alamb This function also will return the timestamp from `ExecutionProps` instead of dynamically generating it. I'm trying to make that change right now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #288: [Datafusion] NOW, CURRENT_TIMESTAMP, CURRENT_TIME functions
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-835710437
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#288](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (32304cf) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/5f6024d570f4d2d5fd4c2187ff7dcb0cd389ac4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5f6024d) will **decrease** coverage by `0.67%`.
> The diff coverage is `89.04%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
- Coverage 76.80% 76.13% -0.68%
==========================================
Files 133 141 +8
Lines 23284 23705 +421
==========================================
+ Hits 17884 18048 +164
- Misses 5400 5657 +257
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZXhlY3V0aW9uL2NvbnRleHQucnM=) | `92.64% <ø> (+0.01%)` | :arrow_up: |
| [...tafusion/src/physical\_plan/datetime\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kYXRldGltZV9leHByZXNzaW9ucy5ycw==) | `67.51% <0.00%> (-1.32%)` | :arrow_down: |
| [datafusion/src/physical\_plan/type\_coercion.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi90eXBlX2NvZXJjaW9uLnJz) | `98.65% <75.00%> (-0.66%)` | :arrow_down: |
| [datafusion/src/optimizer/timestamp\_evaluation.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3RpbWVzdGFtcF9ldmFsdWF0aW9uLnJz) | `93.33% <93.33%> (ø)` | |
| [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9mdW5jdGlvbnMucnM=) | `89.46% <100.00%> (-0.03%)` | :arrow_down: |
| [datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi90ZXN0cy9zcWwucnM=) | `99.89% <100.00%> (+<0.01%)` | :arrow_up: |
| [datafusion/src/physical\_plan/coalesce\_batches.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9jb2FsZXNjZV9iYXRjaGVzLnJz) | `83.18% <0.00%> (-1.77%)` | :arrow_down: |
| [...tafusion/src/physical\_plan/distinct\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9kaXN0aW5jdF9leHByZXNzaW9ucy5ycw==) | `90.34% <0.00%> (-0.57%)` | :arrow_down: |
| [datafusion/src/datasource/csv.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvZGF0YXNvdXJjZS9jc3YucnM=) | `73.07% <0.00%> (-0.26%)` | :arrow_down: |
| [...lista/rust/core/src/serde/logical\_plan/to\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9sb2dpY2FsX3BsYW4vdG9fcHJvdG8ucnM=) | `68.05% <0.00%> (-0.10%)` | :arrow_down: |
| ... and [22 more](https://codecov.io/gh/apache/arrow-datafusion/pull/288/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5f6024d...32304cf](https://codecov.io/gh/apache/arrow-datafusion/pull/288?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
returnString edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840800353
> I also am not sure I would call now() "stateful" in the sense that it has state that changes during the execution of the query. It is more like "parameterized" or something with a parameter that is filled in prior to execution
Yeah, in Postgres parlance this kind of function is "stable" (read: consistent over the course of a single txn given the same inputs) as opposed to "immutable" (which can't reference _anything_ outside its args/constants) - still not the best breakdown imo, but a little bit closer maybe.
Cache invalidation and naming things, right? 😅
> Also, I am not convinced about how valuable a general purpose StatefulFunction will be (though of course it depends on a proposal that is not yet written ;) )
Off the top of my head I think it'll open up some potential for generalised optimisation passes over function usage in queries according to function class, i.e. the optimiser rule used for the initial implementation of this PR but applicable to arbitrary functions provided they indicate themselves to be "stable".
> is it fair to say that there is some urgency in the NOW? In this case, I agree that the closure is a great stop-gap and we should go for it.
Personally I think so, it's a pretty generally useful function and opens up lots of good time-series use cases as @alamb mentioned.
Edit: also, I'm happy to assist in defining this proposed future work or even drive it outright; not just trying to generate tasks for other people ;)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] returnString commented on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
returnString commented on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-839692218
> I am not sure we should do this: we are changing all functions' signatures because of a single function's requirement.
>
> I think we should add a node specifically for `NOW` and leave all the other functions' alone.
We'll probably hit up against similar issues for current_date, current_time, current_timestamp, etc, in case that helps formulate what said node would look like.
Interestingly, this is an instance where it would be much _easier_ if slightly less performant to implement as a UDF for certain use cases, because you could just close over some external state when building your execution ctx.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb edited a comment on pull request #288: [Datafusion] NOW() function support
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #288:
URL: https://github.com/apache/arrow-datafusion/pull/288#issuecomment-840763111
@jorgecarleitao Also, I am not convinced about how valuable a general purpose `StatefulFunction` will be (though of course it depends on a proposal that is not yet written ;) )
This is due to:
1. You can write such a function today as a `ScalarFunction` -- via a closure with an `Arc<...>` with shared state. I don't see much advantage to building that into DataFusion. Window functions I think are going to have to be their own thing (like `SortExpr` and `AggregateExpr`.
1. Given there is no way to know what order DataFusion will pass inputs to your functions (across threads, repartitioned, etc) I can't think of a usecase at this moment 🤔
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org