Add operator section to user guide, Add `std::ops` operations to `prelude`, and add `not()` expr_fn #7732

ongchi · 2023-10-03T14:13:17Z

Which issue does this PR close?

Closes #7681

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

ongchi · 2023-10-04T02:32:57Z

Several functions added in this PR:

Functions already listed in documentation but not implemented:
- not
- eq
- not_eq
- gt
- gt_eq
- lt
- lt_eq
Additional arithmetic functions introduced in this PR:
- add
- sub
- mul
- div
- mod_
- neg

Few more questions:

These implementations and tests already exist somewhere. Do these still have to add test code here?
mod is a reserved keyword in Rust. The reminder function named here to be able consistant with SQL function naming, but does it ok to datafusion?

Edit: change r#mod to mod_ for better code completion

ongchi · 2023-10-04T02:52:56Z

Methods added to Expr in this PR:

Methods already listed in documentation but not implemented:
- bitwise_and
- bitwise_or
- bitwise_xor
- bitwise_shift_left
- bitwise_shift_right
- not
Additional arithmetic methods introduced in this PR:
- As same as arithmetic functions listed above

I found that there are multiple implementations to do the same thing, e.g., bitwise operations on https://github.com/apache/arrow-datafusion/blob/a64f36d8fbce35cb7146271fb5d9ab98a9f094e6/datafusion/expr/src/expr_fn.rs#L172-L215 and https://github.com/apache/arrow-datafusion/blob/a64f36d8fbce35cb7146271fb5d9ab98a9f094e6/datafusion/expr/src/operator.rs#L313-L355, that should/is identical to each other. Do we have to merge these implementations and place tests in the same place?

ongchi · 2023-10-04T09:50:23Z

datafusion/expr/src/expr.rs

+    //
+
+    /// Return `self + other`
+    #[allow(clippy::should_implement_trait)]


Clippy fires warnings because these methods have same name on trait methods in std::ops. Without these methods, user may require to include specific traits from std::ops::* in their source. Or, should we re-export these traits in prelude?

user may require to include specific traits from std::ops::* in their source.

I think using the std::ops in their traits is the less surprising API and is the "idiomatic" rust way to do this. I think we should remove these methods and just use std::ops

Or, should we re-export these traits in prelude?

This would be ok with me. Alternately we could add an example to https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html saying that it is possible to use the std::ops and show an example of a + b or the equivalent

datafusion/expr/src/expr_fn.rs

comphead · 2023-10-04T15:23:40Z

docs/source/conf.py

@@ -118,4 +118,4 @@
 myst_heading_anchors = 3

 # enable nice rendering of checkboxes for the task lists
-myst_enable_extensions = [ "tasklist"]
+myst_enable_extensions = ["colon_fence", "deflist", "tasklist"]


not sure if its related to this PR description 🤔

I add some admonitions and definition list to the page that require colon_fence and deflist extensions.

Like this
https://github.com/apache/arrow-datafusion/blob/da033810b192f24a8fb63fb8e5d509e6469b482a/docs/source/user-guide/expressions.md?plain=1#L47-L50

docs/source/user-guide/expressions.md

comphead · 2023-10-04T15:33:04Z

Thanks @ongchi for the PR, I left some minor comments

comphead · 2023-10-07T19:10:57Z

Doc is definitely looks more solid and consistent now, thanks @ongchi
For the new methods introduced for Expr trait we may need to cover them by unit tests.

Another thing comes to my mind if we support both syntaxes, we should prob have some notification when new operator is introduced.
It might be a common trait that will require during compilation for another syntax to be implemented

ongchi · 2023-10-08T12:11:24Z

Another thing comes to my mind if we support both syntaxes, we should prob have some notification when new operator is introduced. It might be a common trait that will require during compilation for another syntax to be implemented

I think we should reorganize test cases in datafusion-expr and make some improvements by creating a test module. Currently, these tests do not cover all use cases. The tests may follow the order/structure from expression API documentation, which would make it easier to identify which parts needs to be improve.

comphead · 2023-10-09T16:02:24Z

datafusion/expr/src/expr.rs

+    }
+
+    /// Return `self % other`
+    pub fn mod_(self, other: Expr) -> Expr {


underscore in naming might be confusing

comphead · 2023-10-09T16:06:39Z

datafusion/expr/src/expr.rs

+    }
+
+    #[test]
+    fn test_logical_ops() {


awesome. In addition to planner test would be great to have same tests on Dataframe level to prove operators returns correct values.

Update examples and factor out the common interface (to make sure new added operator will be covered by all kind of syntaxes) can be done is follow up PR

alamb

Thank you so much @ongchi and @comphead -- this PR is looking really great. I do think we should switch back to using std::ops but otherwise I think this PR is ready to go.

A really nice addition to the docs and usability in general

alamb · 2023-10-10T17:56:31Z

datafusion/expr/src/expr.rs

+    //
+
+    /// Return `self + other`
+    #[allow(clippy::should_implement_trait)]


user may require to include specific traits from std::ops::* in their source.

I think using the std::ops in their traits is the less surprising API and is the "idiomatic" rust way to do this. I think we should remove these methods and just use std::ops

Or, should we re-export these traits in prelude?

This would be ok with me. Alternately we could add an example to https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.Expr.html saying that it is possible to use the std::ops and show an example of a + b or the equivalent

alamb · 2023-10-10T17:57:43Z

datafusion/expr/src/expr_rewriter/mod.rs

@@ -265,7 +265,6 @@ mod test {
    use arrow::datatypes::DataType;
    use datafusion_common::tree_node::{RewriteRecursion, TreeNode, TreeNodeRewriter};
    use datafusion_common::{DFField, DFSchema, ScalarValue};
-    use std::ops::Add;


Implementing std::ops::Add means you can write something like expr1 + expr2.

alamb · 2023-10-13T17:36:21Z

Converting to draft as this PR is not waiting on feedback anymore. Please mark it ready to review when ready.

I do realize when writing documentation, often deficiencies in the existing APIs become clearer

One suggestion I had was to split out the changes to the documentation from the changes to how the traits work.

I think we should reorganize test cases in datafusion-expr and make some improvements by creating a test module. Currently, these tests do not cover all use cases. The tests may follow the order/structure from expression API documentation, which would make it easier to identify which parts needs to be improve.

I agree -- @ongchi could you please file a follow on ticket describing this work? Perhaps it would be a good first step in updating how the traits worked: consolidate all the existing tests we do have, so the scope of the "to do" work becomes clear.

ongchi · 2023-10-16T14:20:25Z

Converting to draft as this PR is not waiting on feedback anymore. Please mark it ready to review when ready.

I do realize when writing documentation, often deficiencies in the existing APIs become clearer

One suggestion I had was to split out the changes to the documentation from the changes to how the traits work.

I think we should reorganize test cases in datafusion-expr and make some improvements by creating a test module. Currently, these tests do not cover all use cases. The tests may follow the order/structure from expression API documentation, which would make it easier to identify which parts needs to be improve.

I agree -- @ongchi could you please file a follow on ticket describing this work? Perhaps it would be a good first step in updating how the traits worked: consolidate all the existing tests we do have, so the scope of the "to do" work becomes clear.

Sure, I'm happy to do that.

I'll focus on correctness of the documentation in this PR with minimal code changes. Any further changes can depend on other users' input.

…wise, comparison, and arithmetic expressions

alamb

Thank you @ongchi -- I think this one looks good to me. What do you think @comphead ?

github-actions bot added the logical-expr Logical plan and expressions label Oct 3, 2023

ongchi force-pushed the doc-operators branch from 41009a5 to 046e020 Compare October 4, 2023 02:09

ongchi force-pushed the doc-operators branch from 046e020 to fb6f436 Compare October 4, 2023 03:30

ongchi marked this pull request as ready for review October 4, 2023 03:44

ongchi commented Oct 4, 2023

View reviewed changes

comphead reviewed Oct 4, 2023

View reviewed changes

datafusion/expr/src/expr_fn.rs Outdated Show resolved Hide resolved

comphead reviewed Oct 4, 2023

View reviewed changes

docs/source/user-guide/expressions.md Outdated Show resolved Hide resolved

comphead reviewed Oct 4, 2023

View reviewed changes

docs/source/user-guide/expressions.md Outdated Show resolved Hide resolved

comphead reviewed Oct 4, 2023

View reviewed changes

docs/source/user-guide/expressions.md Outdated Show resolved Hide resolved

ongchi force-pushed the doc-operators branch from fb6f436 to 43f9b3f Compare October 7, 2023 13:26

ongchi requested a review from comphead October 7, 2023 13:34

ongchi force-pushed the doc-operators branch from 36b862a to fd95dd6 Compare October 9, 2023 00:28

github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Oct 9, 2023

comphead reviewed Oct 9, 2023

View reviewed changes

alamb reviewed Oct 10, 2023

View reviewed changes

alamb marked this pull request as draft October 13, 2023 17:33

ongchi added 2 commits October 16, 2023 22:02

Add logical tests of Expr and logical not function

692d9bc

Re-export std::ops::* in prelude

d264753

Update the Expression API documentation, mainly covering boolean, bit…

e0ecaf3

…wise, comparison, and arithmetic expressions

ongchi force-pushed the doc-operators branch from fd95dd6 to e0ecaf3 Compare October 16, 2023 14:24

github-actions bot removed the optimizer Optimizer rules label Oct 16, 2023

alamb marked this pull request as ready for review October 16, 2023 15:36

alamb changed the title ~~Add operator section to user guide~~ Add operator section to user guide, Add std::ops operations to prelude, and add not() expr_fn Oct 16, 2023

alamb approved these changes Oct 16, 2023

View reviewed changes

alamb merged commit 360175a into apache:main Oct 17, 2023
24 checks passed

ongchi deleted the doc-operators branch October 23, 2023 11:29

ongchi mentioned this pull request Nov 4, 2023

Minor: Remove the irrelevant note from the Expression API doc #8053

Merged

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add operator section to user guide, Add `std::ops` operations to `prelude`, and add `not()` expr_fn #7732

Add operator section to user guide, Add `std::ops` operations to `prelude`, and add `not()` expr_fn #7732

ongchi commented Oct 3, 2023

ongchi commented Oct 4, 2023 •

edited

Loading

ongchi commented Oct 4, 2023

ongchi Oct 4, 2023

alamb Oct 10, 2023

comphead Oct 4, 2023

ongchi Oct 5, 2023

comphead commented Oct 4, 2023

comphead commented Oct 7, 2023

ongchi commented Oct 8, 2023

comphead Oct 9, 2023

comphead Oct 9, 2023

alamb left a comment

alamb Oct 10, 2023

alamb Oct 10, 2023

alamb commented Oct 13, 2023

ongchi commented Oct 16, 2023

alamb left a comment

Add operator section to user guide, Add std::ops operations to prelude, and add not() expr_fn #7732

Add operator section to user guide, Add std::ops operations to prelude, and add not() expr_fn #7732

Conversation

ongchi commented Oct 3, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

ongchi commented Oct 4, 2023 • edited Loading

ongchi commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead commented Oct 4, 2023

comphead commented Oct 7, 2023

ongchi commented Oct 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Oct 13, 2023

ongchi commented Oct 16, 2023

alamb left a comment

Choose a reason for hiding this comment

Add operator section to user guide, Add `std::ops` operations to `prelude`, and add `not()` expr_fn #7732

Add operator section to user guide, Add `std::ops` operations to `prelude`, and add `not()` expr_fn #7732

ongchi commented Oct 4, 2023 •

edited

Loading