Convert `rank` / `dense_rank` and `percent_rank` builtin functions to UDWF by jatin510 · Pull Request #12718 · apache/datafusion

jatin510 · 2024-10-02T12:43:41Z

Which issue does this PR close?

Closes #12648

Rationale for this change

Same as described in #8709.

What changes are included in this PR?

Converts BuiltinWindowFunction::Rank, BuiltinWindowFunction::DenseRank, BuiltinWindowFunction::PercentRank to user-defined window function.
Export a fluent-style API for creating rank, dense_rank, percent_rank expressions.

Are these changes tested?

Updates explain output in sqllogictests to match lowercase rank, dense_rank and percent_rank.

Are there any user-facing changes?

jatin510 · 2024-10-02T12:45:10Z

datafusion/proto/proto/datafusion.proto

+  // TODO
+  // if i will remove this
+  // should i renumber all variables?
  RANK = 1;


@jcsherin how to handle this ?
I want to remove rank, percent_rank and dense_rank from here ?

You just need to comment out the lines (not remove them):

enum BuiltInWindowFunction { UNSPECIFIED = 0; // https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum // ROW_NUMBER = 0; // RANK = 1; // DENSE_RANK = 2; // PERCENT_RANK = 3;

…elated to Datafusion window function

…functions-window

…d updated pbson fields

jatin510 · 2024-10-04T05:39:16Z

datafusion/sql/tests/sql_integration.rs

        \n    Aggregate: groupBy=[[ROLLUP (person.state, person.last_name)]], aggr=[[sum(person.age), grouping(person.state), grouping(person.last_name)]]\
        \n      TableScan: person";
+    // TODO
+    // fix this


@jcsherin when i am running cargo test,
it is not able to detect the rank, dense_rank and percent_rank udwf functions.
I even added the window_function at line 2637 as well.
Any idea where to register the newly created udwf's.

Thanks, In advance .

The UDWF is registered but the lookup in get_window_meta still returns None.

datafusion/datafusion/sql/tests/common/mod.rs

Lines 220 to 222 in 18193e6

fn get_window_meta(&self, _name: &str) -> Option<Arc<WindowUDF>> {

None

}

This PR is coming along well 😄. Thanks @jatin510.

…rank function

jatin510 · 2024-10-04T19:03:21Z

datafusion/expr/src/expr_fn.rs

+/// how to handle import of percent_rank function from
+/// datafusion+functions_window
+/// as using this is causing the circular depency issue
 /// # // first_value is an aggregate function in another crate


the expr function have written docs for the percent_rank,
but when I try to import the percent_rank from datafusion_functions_window workspace,
its throwing error related to the circular depency

how to handle such cases ?

There are a couple of ways to get around this. But here I noticed that the first_value function is a mock and not the real first_value window function.

So you can consider doing the same for percent_rank. Then you don't have to import anything. This is alright because the test is not testing the functionality of percent_rank in the doc test.

Another advantage of mocking in this specific case is that the change remains local to the doc test which I believe is good.

alamb · 2024-10-04T22:04:29Z

This is so exciting to see

jatin510 · 2024-10-07T05:33:49Z

in window.slt file
while running cargo test
getting error:

Running "map.slt"
External error: query failed: DataFusion error: Arrow error: Invalid argument error: column types must match schema types, expected UInt64 but found Float64 at column index 6
[SQL] SELECT
  c9,
  RANK() OVER(ORDER BY c1) AS rank1,
  RANK() OVER(ORDER BY c1 ROWS BETWEEN 10 PRECEDING and 1 FOLLOWING) as rank2,
  DENSE_RANK() OVER(ORDER BY c1) as dense_rank1,
  DENSE_RANK() OVER(ORDER BY c1 ROWS BETWEEN 10 PRECEDING and 1 FOLLOWING) as dense_rank2,
  PERCENT_RANK() OVER(ORDER BY c1) as percent_rank1,
  PERCENT_RANK() OVER(ORDER BY c1 ROWS BETWEEN 10 PRECEDING and 1 FOLLOWING) as percent_rank2
  FROM aggregate_test_100
  ORDER BY c9
  LIMIT 5
at test_files/window.slt:1151

but percent_rank is supposed to return the float64 value

jcsherin · 2024-10-07T07:47:45Z

but percent_rank is supposed to return the float64 value

~~The built-in implementation doesn't define the result type as Datatype::Int64. It depends on the type of the input expression.~~

datafusion/datafusion/physical-expr/src/window/rank.rs

Lines 95 to 98 in a1ae158

    
           fn field(&self) -> Result<Field> { 
        
               let nullable = false; 
        
               Ok(Field::new(self.name(), self.data_type.clone(), nullable)) 
        
           }

~~You can use the WindowUDFFieldArgs::get_input_type method to set it.~~

datafusion/datafusion/functions-window-common/src/field.rs

Lines 59 to 63 in e1b992a

    
               /// Returns `Some(DataType)` of input expression at index, otherwise 
        
               /// returns `None` if the index is out of bounds. 
        
               pub fn get_input_type(&self, index: usize) -> Option<DataType> { 
        
                   self.input_types.get(index).cloned() 
        
               }

@jatin510 You are right, the return type is DataType::Float64 for percent_rank. Your fix in f1c7547 looks right to me.

The percent_rank takes zero arguments, so it does not depend on the type of the input expression.

The return_type for the BuiltInWindowFunction is defined here:

datafusion/datafusion/expr/src/built_in_window_function.rs

Lines 129 to 135 in 389f7f7

    
           match self { 
        
               BuiltInWindowFunction::Rank 
        
               | BuiltInWindowFunction::DenseRank 
        
               | BuiltInWindowFunction::Ntile => Ok(DataType::UInt64), 
        
               BuiltInWindowFunction::PercentRank | BuiltInWindowFunction::CumeDist => { 
        
                   Ok(DataType::Float64) 
        
               }

jatin510 · 2024-10-08T04:44:05Z

Thanks for your help, @jcsherin

jcsherin · 2024-10-08T12:46:35Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

You can add test cases for the expression API here:

datafusion/datafusion/proto/tests/cases/roundtrip_logical_plan.rs

Line 803 in ceba9d4

async fn roundtrip_expr_api() -> Result<()> {

jcsherin · 2024-10-08T12:49:36Z

datafusion/sql/tests/common/mod.rs

+
+    pub fn with_window_function(mut self, window_function: Arc<WindowUDF>) -> Self {
+        self.window_functions.insert(
+            window_function.name().to_string().to_lowercase(),


I don't think the to_lowercase() is needed here for WindowUDFs.

jcsherin · 2024-10-08T13:09:51Z

@jatin510 Great job! This PR is almost there ✊.

The Rank user-defined window function implementation is now split across three modules. This has resulted in duplicated code in many places.

It would be nicer if we continue to maintain the same form factor as the original implementation.

You can reuse the RankType enum used in the built-in window function implementation in WindowUDF:

pub enum RankType {
    Basic,
    Dense,
    Percent,
}

And in PartitionEvaluator::evaluate you can specialize like this,

/// Evaluates the window function inside the given range.
    fn evaluate(
        &mut self,
        values: &[ArrayRef],
        range: &Range<usize>,
    ) -> Result<ScalarValue> {

      /// no more code duplication here ...

       match self.rank_type {
            RankType::Basic => Ok(ScalarValue::UInt64(Some(
                self.state.last_rank_boundary as u64 + 1,
            ))),
            RankType::Dense => Ok(ScalarValue::UInt64(Some(self.state.n_rank as u64))),
            RankType::Percent => {
                exec_err!("Can not execute PERCENT_RANK in a streaming fashion")
            }
        }
}

Would you like to unify this? If not, this can be completed in a follow-on PR.

jcsherin · 2024-10-08T13:16:00Z

datafusion/functions-window/src/dense_rank.rs

+define_udwf_and_expr!(
+    DenseRank,
+    dense_rank,
+    "Returns the rank of each row within a window partition without gaps."


Suggested change

"Returns the rank of each row within a window partition without gaps."

"Returns rank of the current row without gaps. This function counts peer groups"

See https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.BuiltInWindowFunction.html#variant.DenseRank

jcsherin · 2024-10-08T13:17:34Z

datafusion/functions-window/src/percent_rank.rs

+define_udwf_and_expr!(
+    PercentRank,
+    percent_rank,
+    "Returns the relative rank of the current row within a window partition."


Suggested change

"Returns the relative rank of the current row within a window partition."

"Returns the relative rank of the current row: (rank - 1) / (total rows - 1)"

See https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.BuiltInWindowFunction.html#variant.PercentRank

datafusion/functions-window/src/rank.rs

jatin510 · 2024-10-08T14:40:05Z

@jatin510 Great job! This PR is almost there ✊.

The Rank user-defined window function implementation is now split across three modules. This has resulted in duplicated code in many places.

It would be nicer if we continue to maintain the same form factor as the original implementation.

You can reuse the RankType enum used in the built-in window function implementation in WindowUDF:
pub enum RankType {
    Basic,
    Dense,
    Percent,
}
And in PartitionEvaluator::evaluate you can specialize like this,
/// Evaluates the window function inside the given range.
    fn evaluate(
        &mut self,
        values: &[ArrayRef],
        range: &Range<usize>,
    ) -> Result<ScalarValue> {

      /// no more code duplication here ...

       match self.rank_type {
            RankType::Basic => Ok(ScalarValue::UInt64(Some(
                self.state.last_rank_boundary as u64 + 1,
            ))),
            RankType::Dense => Ok(ScalarValue::UInt64(Some(self.state.n_rank as u64))),
            RankType::Percent => {
                exec_err!("Can not execute PERCENT_RANK in a streaming fashion")
            }
        }
}
Would you like to unify this? If not, this can be completed in a follow-on PR.

I will create a follow up PR for this change

jcsherin · 2024-10-08T14:56:39Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

+        rank(),
+        dense_rank(),
+        percent_rank(),


jatin510 · 2024-10-08T18:34:48Z

The PR is ready to be reviewed @jcsherin @alamb

alamb · 2024-10-08T20:57:16Z

Awesome -- than you @jatin510 -- I have startd the CI and plan to review this tomorrow morning

…nk-percentrank-denserank

alamb

Thank you @jatin510 and @jcsherin -- this is a really nice improvement. We are getting very close to having no built in functions 🎉 .

FYI @mustafasrepo @ozankabak and @berkaysynnada

alamb · 2024-10-09T16:49:12Z

datafusion/functions-window/src/dense_rank.rs

+// specific language governing permissions and limitations
+// under the License.
+
+//! Defines physical expression for `dense_rank` that can evaluated at runtime during query execution


Technically speaking this module contains the WindowUDFImpl for dense_rank, but I think this wording is consistent with the other code in this crate.

I have this PR checked out to resolve some logical conflicts anyways, so I will pushed a commit to change it in this PR too

jatin510 · 2024-10-09T17:06:46Z

Thanks, @alamb and @jcsherin, for all your help! You both are awesome!

alamb · 2024-10-10T20:28:38Z

🚀

wip: converting rank builtin function to UDWF

05fd8bc

jatin510 marked this pull request as draft October 2, 2024 12:43

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate proto Related to proto crate labels Oct 2, 2024

jatin510 commented Oct 2, 2024

View reviewed changes

jatin added 6 commits October 2, 2024 20:31

commented BuiltInWindowFunction in datafusion.proto and fixed issue r…

65d213c

…elated to Datafusion window function

implemented rank.rs, percent_rank.rs and dense_rank.rs in datafusion …

d7f38fa

…functions-window

removed a test from built in window function test for percent_rank an…

00e9044

…d updated pbson fields

Merge branch 'main' into feature/12648-udwf-rank-percentrank-denserank

ceba9d4

removed unnecessary code

a3a4589

added window_functions field to the MockSessionState

5a401ff

github-actions bot added the sql SQL Planner label Oct 3, 2024

jatin510 added 3 commits October 4, 2024 09:40

merge main branch

04085bb

updated rank, percent_rank and dense_rank udwf to use macros

07eb07f

wip: fix rank functionality in sql integration

937c030

jatin510 commented Oct 4, 2024

View reviewed changes

jatin510 added 4 commits October 4, 2024 15:13

fixed rank udwf not found issue in sql_integration.rs

f06bdc7

evaluating rank, percent_rank and dense_rank udwf with evaluate_with_…

736fd6e

…rank function

fixed rank projection test

ceb594b

wip: fixing the percent_rank() documentation

98dfbbf

github-actions bot added sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate labels Oct 4, 2024

jatin510 commented Oct 4, 2024

View reviewed changes

fixed the docs error issue

f57c5d2

fixed data type of the percent_rank udwf

f1c7547

jatin510 marked this pull request as ready for review October 8, 2024 04:00

updated prost.rs file

b9613e1

jatin510 changed the title ~~wip: converting rank builtin function to UDWF~~ Converting rank builtin function to UDWF Oct 8, 2024

jatin510 requested a review from jcsherin October 8, 2024 07:22

jcsherin reviewed Oct 8, 2024

View reviewed changes

datafusion/functions-window/src/rank.rs Outdated Show resolved Hide resolved

updated test and documentation

bd852e0

jatin510 requested a review from jcsherin October 8, 2024 14:40

jcsherin reviewed Oct 8, 2024

View reviewed changes

alamb changed the title ~~Converting rank builtin function to UDWF~~ Convert rank builtin function to UDWF Oct 9, 2024

alamb added the api change Changes the API exposed to users of the crate label Oct 9, 2024

Merge remote-tracking branch 'apache/main' into feature/12648-udwf-ra…

7ec1604

…nk-percentrank-denserank

alamb approved these changes Oct 9, 2024

View reviewed changes

alamb added 2 commits October 9, 2024 12:54

Fix logical conflicts

0c5d5be

tweak module documentation

38fc8ed

alamb changed the title ~~Convert rank builtin function to UDWF~~ Convert rank / dense_rank and percen_rank builtin functions to UDWF Oct 9, 2024

jatin510 changed the title ~~Convert rank / dense_rank and percen_rank builtin functions to UDWF~~ Convert rank / dense_rank and percent_rank builtin functions to UDWF Oct 10, 2024

alamb merged commit 939ef9e into apache:main Oct 10, 2024

	fn get_window_meta(&self, _name: &str) -> Option<Arc<WindowUDF>> {
	None
	}

	"Returns the rank of each row within a window partition without gaps."
	"Returns rank of the current row without gaps. This function counts peer groups"

	"Returns the relative rank of the current row within a window partition."
	"Returns the relative rank of the current row: (rank - 1) / (total rows - 1)"

Comments

Conversation

jatin510 commented Oct 2, 2024 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsherin Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 4, 2024

Uh oh!

jatin510 commented Oct 7, 2024

Uh oh!

jcsherin commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jatin510 commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsherin commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcsherin Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jatin510 commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin510 commented Oct 8, 2024

Uh oh!

alamb commented Oct 8, 2024

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatin510 commented Oct 9, 2024

Uh oh!

alamb commented Oct 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jatin510 commented Oct 2, 2024 •

edited by alamb

Loading

jcsherin Oct 4, 2024 •

edited

Loading

jcsherin commented Oct 7, 2024 •

edited

Loading

jcsherin commented Oct 8, 2024 •

edited

Loading

jcsherin Oct 8, 2024 •

edited

Loading