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/12 10:16:23 UTC

[GitHub] [arrow-datafusion] Jimexist opened a new pull request #328: fix 305 by using a null array as param for zero param functions

Jimexist opened a new pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328


   # Which issue does this PR close?
   
   fix 305 by using a null array as param for zero param functions
   
   Closes #305
   
    # Rationale for this change
   
   in case of a zero param function we'll have a rather hacky way to pass in the batch num_rows param so that the functions wrapped can be generative the correct result
   
   # What changes are included in this PR?
   
   Similar to #307 but use a null array instead
   
   # Are there any user-facing changes?
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   
   <!--
   If there are any breaking changes to public APIs, please add the `breaking change` label.
   -->
   


-- 
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 #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#discussion_r632736152



##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -1358,6 +1366,18 @@ impl fmt::Display for ScalarFunctionExpr {
     }
 }
 
+/// null columnar values are implemented as a null array in order to pass batch
+/// num_rows
+type NullColumnarValue = ColumnarValue;
+
+impl TryFrom<&RecordBatch> for NullColumnarValue {
+    type Error = DataFusionError;
+    fn try_from(batch: &RecordBatch) -> Result<Self> {

Review comment:
       It seems to me this function is infallible (aka it never returns error, but always returns `Ok`).
   
   Perhaps we could implement `From` instead of `TryFrom`?
   
   




-- 
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] Jimexist commented on a change in pull request #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#discussion_r632883017



##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -1358,6 +1366,18 @@ impl fmt::Display for ScalarFunctionExpr {
     }
 }
 
+/// null columnar values are implemented as a null array in order to pass batch
+/// num_rows
+type NullColumnarValue = ColumnarValue;
+
+impl TryFrom<&RecordBatch> for NullColumnarValue {
+    type Error = DataFusionError;
+    fn try_from(batch: &RecordBatch) -> Result<Self> {

Review comment:
       updated




-- 
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 #328: Use NullArray to Pass row count to ScalarFunctions that take 0 arguments

Posted by GitBox <gi...@apache.org>.
alamb merged pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328


   


-- 
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 #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#issuecomment-841756124


   # [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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 [#328](https://codecov.io/gh/apache/arrow-datafusion/pull/328?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fe4a6df) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/1702d6c85ebfdbc968b1dc427a9799e74b64ff96?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1702d6c) will **increase** coverage by `0.00%`.
   > The diff coverage is `90.90%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-datafusion/pull/328/graphs/tree.svg?width=650&height=150&src=pr&token=JXwWBKD3D9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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     #328   +/-   ##
   =======================================
     Coverage   75.72%   75.72%           
   =======================================
     Files         143      143           
     Lines       23832    23840    +8     
   =======================================
   + Hits        18046    18054    +8     
     Misses       5786     5786           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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/udf.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/328/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi91ZGYucnM=) | `78.26% <ø> (ø)` | |
   | [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/328/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.60% <90.90%> (+0.05%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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/328?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 [1702d6c...fe4a6df](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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] alamb commented on pull request #328: Use NullArray to Pass row count to ScalarFunctions that take 0 arguments

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#issuecomment-841795093


   I agree -- thanks again @Jimexist  -- great stuff


-- 
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 #328: Use NullArray to Pass row count to ScalarFunctions that take 0 arguments

Posted by GitBox <gi...@apache.org>.
alamb merged pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328


   


-- 
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] Jimexist commented on a change in pull request #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#discussion_r633031732



##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -207,6 +215,14 @@ pub enum BuiltinScalarFunction {
     RegexpMatch,
 }
 
+impl BuiltinScalarFunction {
+    /// an allowlist of functions to take zero arguments, so that they will get special treatment
+    /// while executing.
+    fn supports_zero_argument(&self) -> bool {

Review comment:
       had to add this whitelist because vararg funcs like `array` will not accept 0 args




-- 
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 #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#discussion_r630926134



##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -1373,20 +1393,24 @@ impl PhysicalExpr for ScalarFunctionExpr {
     }
 
     fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
-        // evaluate the arguments
-        let inputs = self
-            .args
-            .iter()
-            .map(|e| e.evaluate(batch))
-            .collect::<Result<Vec<_>>>()?;
+        // evaluate the arguments, if there are no arguments we'll instead pass in an uint32 holding

Review comment:
       this comment is now out of date as well




-- 
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 #328: Use NullArray to Pass row count to ScalarFunctions that take 0 arguments

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#issuecomment-841795093


   I agree -- thanks again @Jimexist  -- great stuff


-- 
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] Jimexist commented on a change in pull request #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
Jimexist commented on a change in pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#discussion_r633031732



##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -207,6 +215,14 @@ pub enum BuiltinScalarFunction {
     RegexpMatch,
 }
 
+impl BuiltinScalarFunction {
+    /// an allowlist of functions to take zero arguments, so that they will get special treatment
+    /// while executing.
+    fn supports_zero_argument(&self) -> bool {

Review comment:
       had to add this whitelist because vararg funcs like `array` will not accept 0 args




-- 
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 #328: fix 305 by using a null array as param for zero param functions

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #328:
URL: https://github.com/apache/arrow-datafusion/pull/328#issuecomment-841756124


   # [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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 [#328](https://codecov.io/gh/apache/arrow-datafusion/pull/328?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fe4a6df) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/1702d6c85ebfdbc968b1dc427a9799e74b64ff96?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1702d6c) will **increase** coverage by `0.00%`.
   > The diff coverage is `90.90%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-datafusion/pull/328/graphs/tree.svg?width=650&height=150&src=pr&token=JXwWBKD3D9&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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     #328   +/-   ##
   =======================================
     Coverage   75.72%   75.72%           
   =======================================
     Files         143      143           
     Lines       23832    23840    +8     
   =======================================
   + Hits        18046    18054    +8     
     Misses       5786     5786           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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/udf.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/328/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-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi91ZGYucnM=) | `78.26% <ø> (ø)` | |
   | [datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/328/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.60% <90.90%> (+0.05%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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/328?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 [1702d6c...fe4a6df](https://codecov.io/gh/apache/arrow-datafusion/pull/328?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