Convert `ntile` builtIn function to UDWF #13040

jatin510 · 2024-10-21T17:08:33Z

Which issue does this PR close?

Rationale for this change

Same as described in #8709.

What changes are included in this PR?

Converts BuiltinWindowFunction::Ntile to user-defined window function.
Export a fluent-style API for creating ntile expressions.

Are these changes tested?

Yes

Are there any user-facing changes?

jcsherin · 2024-10-22T10:35:26Z

datafusion/functions-window/src/ntile.rs

+    /// Create a new `ntile` function
+    pub fn new() -> Self {
+        Self {
+            signature: Signature::any(0, Volatility::Immutable),


The ntile(expression) window function takes a single argument of the integer type.

Suggested change

signature: Signature::any(0, Volatility::Immutable),

signature: Signature::uniform(

1,

vec![

DataType::UInt64,

DataType::UInt32,

DataType::UInt16,

DataType::UInt8,

DataType::Int64,

DataType::Int32,

DataType::Int16,

DataType::Int8,

],

Volatility::Immutable,

)

Just noticed this is a draft PR 😅. Let me know once it's ready for review.

jcsherin · 2024-10-22T10:42:21Z

Closes #12649.

Convert BuiltInWindowFunction::Ntile to a user defined window function #12694

The issue number for BuiltInWindowFunction::Ntile is #12694.

datafusion/functions-window/src/ntile.rs

jcsherin · 2024-10-22T18:27:39Z

datafusion/functions-window/src/ntile.rs

+        let nullable = false;
+        let return_type: &DataType = field_args.input_types().first().unwrap();
+
+        Ok(Field::new(self.name(), return_type.clone(), nullable))


When constructing the result field (column) we need to pass the fully qualified name in the schema which is available in WindowUDFFieldArgs.

datafusion/datafusion/expr/src/udwf.rs

Lines 387 to 390 in 818ce3f

/// Call `field_args.name()` to get the fully qualified name for defining

/// the [`Field`]. For a complete example see the implementation in the

/// [Basic Example](WindowUDFImpl#basic-example) section.

fn field(&self, field_args: WindowUDFFieldArgs) -> Result<Field>;

It is easy to get confused with the name of the udwf 😅.

External error: query failed: DataFusion error: Internal error: Input field name ntile does not match with the projection expression ntile(Int64(8)) ORDER BY [aggregate_test_100.c4 ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW. This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker [SQL] SELECT NTILE(8) OVER (ORDER BY C4) as ntile1, NTILE(12) OVER (ORDER BY C12 DESC) as ntile2 FROM aggregate_test_100 ORDER BY c7 LIMIT 5 at test_files/window.slt:743

Suggested change

Ok(Field::new(self.name(), return_type.clone(), nullable))

Ok(Field::new(field_args.name(), return_type.clone(), nullable))

jatin510 · 2024-10-24T04:53:59Z

@jcsherin

should i remove this duplicate code from : datafusion/physical-plan/src/windows/mod.rs:
because now we have made copy of these in : datafusion/functions-window/src/utils.rs

warning: function `get_scalar_value_from_args` is never used

   --> datafusion/physical-plan/src/windows/mod.rs:168:4
    |
168 | fn get_scalar_value_from_args(
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: function `get_unsigned_integer` is never used
   --> datafusion/physical-plan/src/windows/mod.rs:199:4
    |
199 | fn get_unsigned_integer(value: ScalarValue) -> Result<u64> {
    |    ^^^^^^^^^^^^^^^^^^^^

jcsherin · 2024-10-24T05:15:10Z

@jcsherin

should i remove this duplicate code from : datafusion/physical-plan/src/windows/mod.rs: because now we have made copy of these in : datafusion/functions-window/src/utils.rs

warning: function `get_scalar_value_from_args` is never used

   --> datafusion/physical-plan/src/windows/mod.rs:168:4
    |
168 | fn get_scalar_value_from_args(
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

warning: function `get_unsigned_integer` is never used
   --> datafusion/physical-plan/src/windows/mod.rs:199:4
    |
199 | fn get_unsigned_integer(value: ScalarValue) -> Result<u64> {
    |    ^^^^^^^^^^^^^^^^^^^^

Yes, please.

jcsherin · 2024-10-24T06:21:29Z

datafusion/functions-window/src/ntile.rs

+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+


Add a inner doc comment:

Suggested change

//! `ntile` window function implementation

jcsherin · 2024-10-24T06:24:30Z

datafusion/functions-window/src/ntile.rs

+pub fn ntile(arg: i64) -> datafusion_expr::Expr {
+    ntile_udwf().call(vec![arg.lit()])
+}


The expression API changed here. The arg type is Expr in the existing API.

https://docs.rs/datafusion/latest/datafusion/logical_expr/window_function/fn.ntile.html

Revert to existing API contract

Add a logical plan roundtrip test

Marked as completed.

jcsherin · 2024-10-24T06:26:46Z

datafusion/functions-window/src/ntile.rs

+    fn expressions(&self, expr_args: ExpressionArgs) -> Vec<Arc<dyn PhysicalExpr>> {
+        parse_expr(expr_args.input_exprs(), expr_args.input_types())
+            .into_iter()
+            .collect::<Vec<_>>()
+    }


Is this necessary?

i took the reference from to be consistent while taking input
lead_lag.rs file

https://github.com/apache/datafusion/pull/12857/files#diff-2a93bb37ea4a10bfb23380b8bc8b94e0b6b103053d7eb082933469caeeca459a

I think you can remove this completely and rely on the default implementation for ntile. From what I see ntile doesn't depend on the input expression in partition evaluator.

This implementation is required in lead/lag because it is possible that it can be called with NULL (input expression), and we try to deduce the type for that NULL if a default value argument exists. For lead/lag the types of the input expression and the default value needs to be the same in partition evaluator.

jcsherin · 2024-10-24T06:43:57Z

datafusion/functions-window/src/ntile.rs

+        let n = get_unsigned_integer(scalar_n)?;
+        if n == 0 {
+            return exec_err!("NTILE requires a positive integer");
+        }
+
+        Ok(Box::new(NtileEvaluator { n }))


Let's keep the original code from create_built_in_window_expr for parsing n unmodified?

Suggested change

let n = get_unsigned_integer(scalar_n)?;

if n == 0 {

return exec_err!("NTILE requires a positive integer");

}

Ok(Box::new(NtileEvaluator { n }))

if scalar_n.is_unsigned() {

let n = get_unsigned_integer(scalar_n)?;

Ok(Box::new(NtileEvaluator { n }))

} else {

let n: i64 = get_signed_integer(scalar_n)?;

if n <= 0 {

return exec_err!("NTILE requires a positive integer");

}

Ok(Box::new(NtileEvaluator { n: n as u64 }))

}

jcsherin · 2024-10-24T06:44:30Z

datafusion/functions-window/src/ntile.rs

+    fn documentation(&self) -> Option<&Documentation> {
+        Some(get_ntile_doc())
+    }


jcsherin · 2024-10-24T06:46:23Z

datafusion/functions-window/src/utils.rs

@@ -51,3 +51,18 @@ pub(crate) fn get_scalar_value_from_args(
        None
    })
 }
+
+pub(crate) fn get_unsigned_integer(value: ScalarValue) -> Result<u64> {


Maybe it is better to extract this function inside ntile.

jcsherin · 2024-10-24T06:46:49Z

datafusion/physical-plan/src/windows/mod.rs

@@ -165,25 +163,6 @@ fn window_expr_from_aggregate_expr(
    }
 }

-fn get_scalar_value_from_args(


jcsherin · 2024-10-24T06:47:36Z

docs/source/user-guide/sql/window_functions.md

-## Ranking functions
-
- [rank](#rank)
- [dense_rank](#dense_rank)
- [ntile](#ntile)
-
-### `rank`
-
-Rank of the current row with gaps; same as row_number of its first peer.
-
-```sql
-rank()
-```
-
-### `dense_rank`
-
-Rank of the current row without gaps; this function counts peer groups.
-
-```sql
-dense_rank()
-```
-
-### `ntile`
-
-Integer ranging from 1 to the argument value, dividing the partition as equally as possible.
-
-```sql
-ntile(expression)
-```
-
-#### Arguments
-
- **expression**: An integer describing the number groups the partition should be split into
-


These docs should not be removed?

i have moved them to docs/source/user-guide/sql/window_functions_new.md
Should I still keep them here

This makes sense to me now. You are removing the {rank, dense_rank} functions here because they were already migrated to new documentation.

Let this be 👍

jcsherin · 2024-10-24T06:47:52Z

docs/source/user-guide/sql/window_functions_new.md

@@ -179,6 +180,18 @@ Returns the rank of the current row without gaps. This function ranks rows in a
 dense_rank()
 ```

+### `ntile`


jayzhan211

👍

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

jcsherin · 2024-10-24T14:50:12Z

@jatin510 Can you please mark this PR as draft so that it is not accidentally merged into main while you are working on changes. You can change it back when you are ready, so the committers will know it is ready to merge.

jcsherin · 2024-10-24T14:52:09Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

@@ -940,6 +940,7 @@ async fn roundtrip_expr_api() -> Result<()> {
            vec![lit(1), lit(2), lit(3)],
            vec![lit(10), lit(20), lit(30)],
        ),
+        cume_dist(),


jcsherin · 2024-10-24T14:56:06Z

datafusion/functions-window/src/ntile.rs

+pub fn ntile(arg: Expr) -> Expr {
+    ntile_udwf().call(vec![arg])
+}


jcsherin · 2024-10-24T15:39:22Z

@jatin510 Thanks 🙌.

The udwf epic is almost complete!

jatin510 · 2024-10-24T15:50:13Z

@jcsherin Oops !
I accidentally worked on ntile udwf, I was supposed to work on nth udwf.
Also the branch name is wrong : jatin510:feature/12649-udwf-nth-value

Should I delete this remote branch and update the branch name and create a PR again !

jcsherin · 2024-10-24T15:59:35Z

@jcsherin Oops ! I accidentally worked on ntile udwf, I was supposed to work on nth udwf. Also the branch name is wrong : jatin510:feature/12649-udwf-nth-value

Should I delete this remote branch and update the branch name and create a PR again !

It is alright, that is not a problem at all 😂.

You just need to edit the Closes # in the PR description to refer instead to the ntile issue.

Convert BuiltInWindowFunction::Ntile to a user defined window function #12694

The nth_value conversion is also a good first issue which I hope somebody who wishes to gain experience with DataFusion will pick up soon.

jatin510 · 2024-10-24T16:48:13Z

Thank you so much ! @jcsherin

alamb · 2024-10-24T20:34:27Z

Is this PR ready to go? It looks so to me. Thank you @jatin510 and @jcsherin and @jayzhan211

jcsherin · 2024-10-25T04:40:04Z

@alamb This PR is ready.

alamb · 2024-10-25T13:12:54Z

Thanks again @jatin510 for this PR, @jcsherin for the guidance and @jayzhan211 for the review 🚀

jatin510 added 2 commits October 21, 2024 19:32

converting to ntile udwf

3453f62

updated the window functions documentation file

77ca62f

github-actions bot added documentation Improvements or additions to documentation logical-expr Logical plan and expressions physical-expr Physical Expressions proto Related to proto crate labels Oct 21, 2024

jcsherin reviewed Oct 22, 2024

View reviewed changes

wip: update the ntile udwf function

7d31d6e

jatin510 added 2 commits October 22, 2024 16:41

fix the roundtrip_logical_plan.rs

9db4f17

removed builtIn ntile function

04a0ecc

jatin510 commented Oct 22, 2024

View reviewed changes

datafusion/functions-window/src/ntile.rs Show resolved Hide resolved

jcsherin reviewed Oct 22, 2024

View reviewed changes

jatin510 added 8 commits October 23, 2024 13:29

fixed field name issue

5f907f1

merging main branch

18e34a4

fixing the return type of ntile udwf

251f073

error if UInt64 conversion fails

121bf10

handling if null is found

9e28d9c

handling if value is zero or less than zero

91024c8

removed unused import

cfe533a

updated prost.rs file

c809033

jatin510 added 2 commits October 24, 2024 11:07

removed dead code

d4d1e5b

fixed clippy error

da3c119

jatin510 marked this pull request as ready for review October 24, 2024 05:49

jcsherin reviewed Oct 24, 2024

View reviewed changes

added inner doc comment

8c5d8c8

jcsherin reviewed Oct 24, 2024

View reviewed changes

jayzhan211 approved these changes Oct 24, 2024

View reviewed changes

jcsherin reviewed Oct 24, 2024

View reviewed changes

datafusion/proto/tests/cases/roundtrip_logical_plan.rs Outdated Show resolved Hide resolved

minor fixes and added roundtrip logical plan test

d7e183c

jcsherin reviewed Oct 24, 2024

View reviewed changes

removed parse_expr in ntile

decb133

jatin510 marked this pull request as draft October 24, 2024 15:27

jatin510 marked this pull request as ready for review October 24, 2024 16:47

alamb merged commit 02b9693 into apache:main Oct 25, 2024
25 checks passed

alamb mentioned this pull request Oct 29, 2024

Oct 28, 2024: This week in DataFusion #13167

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `ntile` builtIn function to UDWF #13040

Convert `ntile` builtIn function to UDWF #13040

jatin510 commented Oct 21, 2024 •

edited

Loading

jcsherin Oct 22, 2024

jcsherin Oct 22, 2024

jcsherin commented Oct 22, 2024

jcsherin Oct 22, 2024

jatin510 commented Oct 24, 2024 •

edited

Loading

jcsherin commented Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024 •

edited

Loading

jcsherin Oct 24, 2024

jatin510 Oct 24, 2024

jcsherin Oct 24, 2024 •

edited

Loading

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jatin510 Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jayzhan211 left a comment

jcsherin commented Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin Oct 24, 2024

jcsherin commented Oct 24, 2024

jatin510 commented Oct 24, 2024

jcsherin commented Oct 24, 2024 •

edited

Loading

jatin510 commented Oct 24, 2024 •

edited

Loading

alamb commented Oct 24, 2024 •

edited

Loading

jcsherin commented Oct 25, 2024

alamb commented Oct 25, 2024

-            signature: Signature::any(0, Volatility::Immutable),
+            signature: Signature::uniform(
+,
+                vec![
+                    DataType::UInt64,
+                    DataType::UInt32,
+                    DataType::UInt16,
+                    DataType::UInt8,
+                    DataType::Int64,
+                    DataType::Int32,
+                    DataType::Int16,
+                    DataType::Int8,
+                ],
+                Volatility::Immutable,
+            )

	/// Call `field_args.name()` to get the fully qualified name for defining
	/// the [`Field`]. For a complete example see the implementation in the
	/// [Basic Example](WindowUDFImpl#basic-example) section.
	fn field(&self, field_args: WindowUDFFieldArgs) -> Result<Field>;

	Ok(Field::new(self.name(), return_type.clone(), nullable))
	Ok(Field::new(field_args.name(), return_type.clone(), nullable))

Convert ntile builtIn function to UDWF #13040

Convert ntile builtIn function to UDWF #13040

Conversation

jatin510 commented Oct 21, 2024 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsherin commented Oct 22, 2024

Choose a reason for hiding this comment

jatin510 commented Oct 24, 2024 • edited Loading

jcsherin commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsherin Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsherin Oct 24, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

jcsherin commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsherin commented Oct 24, 2024

jatin510 commented Oct 24, 2024

jcsherin commented Oct 24, 2024 • edited Loading

jatin510 commented Oct 24, 2024 • edited Loading

alamb commented Oct 24, 2024 • edited Loading

jcsherin commented Oct 25, 2024

alamb commented Oct 25, 2024

Convert `ntile` builtIn function to UDWF #13040

Convert `ntile` builtIn function to UDWF #13040

jatin510 commented Oct 21, 2024 •

edited

Loading

jatin510 commented Oct 24, 2024 •

edited

Loading

jcsherin Oct 24, 2024 •

edited

Loading

jcsherin Oct 24, 2024 •

edited

Loading

jcsherin commented Oct 24, 2024 •

edited

Loading

jatin510 commented Oct 24, 2024 •

edited

Loading

alamb commented Oct 24, 2024 •

edited

Loading