Skip to content

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented Jun 28, 2021

Which issue does this PR close?

add integration tests for rank, dense_rank, fix last_value evaluation with rank

Closes follow ups on #555

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@jimexist jimexist force-pushed the add-integration-test-rank branch from 091ca96 to c6e4fd0 Compare June 28, 2021 09:21
@jimexist jimexist force-pushed the add-integration-test-rank branch 2 times, most recently from 10e07a9 to 7049b4e Compare June 28, 2021 13:01
@jimexist jimexist marked this pull request as draft June 28, 2021 14:18
@jimexist jimexist force-pushed the add-integration-test-rank branch 2 times, most recently from 8a90c61 to 617190d Compare June 29, 2021 00:46
@jimexist jimexist changed the title add integration tests for rank, dense_rank add integration tests for rank, dense_rank, fix last_value evaluation with rank Jun 29, 2021
@jimexist jimexist force-pushed the add-integration-test-rank branch 2 times, most recently from 5f166a0 to c3b12c1 Compare June 29, 2021 01:58
order by c9 \
limit 5";
let actual = execute(&mut ctx, sql).await;
let expected = vec![
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-[ RECORD 1 ]-----------------------
c9          | 28774375
sum         | 61035129
avg         | 61035129.000000000000
count       | 1
max         | 61035129
min         | 61035129
first_value | 61035129
last_value  | 61035129
nth_value   | ¤
-[ RECORD 2 ]-----------------------
c9          | 63044568
sum         | -47938237
avg         | -23969118.500000000000
count       | 2
max         | 61035129
min         | -108973366
first_value | 61035129
last_value  | -108973366
nth_value   | -108973366
-[ RECORD 3 ]-----------------------
c9          | 141047417
sum         | 575165281
avg         | 191721760.33333333
count       | 3
max         | 623103518
min         | -108973366
first_value | 61035129
last_value  | 623103518
nth_value   | -108973366
-[ RECORD 4 ]-----------------------
c9          | 141680161
sum         | -1352462829
avg         | -338115707.25000000
count       | 4
max         | 623103518
min         | -1927628110
first_value | 61035129
last_value  | -1927628110
nth_value   | -108973366
-[ RECORD 5 ]-----------------------
c9          | 145294611
sum         | -3251637940
avg         | -650327588.00000000
count       | 5
max         | 623103518
min         | -1927628110
first_value | 61035129
last_value  | -1899175111
nth_value   | -108973366

@jimexist jimexist force-pushed the add-integration-test-rank branch from c3b12c1 to 724b2bf Compare June 29, 2021 02:17
@jimexist jimexist force-pushed the add-integration-test-rank branch from 724b2bf to 5b68cbb Compare June 29, 2021 02:26
@jimexist jimexist marked this pull request as ready for review June 29, 2021 02:26
@jimexist
Copy link
Member Author

@alamb and @Dandandan this pull request is ready now

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Nice work @jimexist

.iter()
.map(|range| {
let len = range.end - range.start;
let value = ScalarValue::try_from_array(arr, range.end - 1)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is very cool

// because the default window frame is between unbounded preceding and current
// row, hence the shift because for values with indices < index they should be
// null. This changes when window frames other than default is implemented
shift(arr.as_ref(), index as i64).map_err(DataFusionError::ArrowError)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@alamb alamb merged commit cab3a98 into apache:master Jun 30, 2021
@jimexist jimexist deleted the add-integration-test-rank branch July 3, 2021 03:14
andygrove added a commit to andygrove/datafusion that referenced this pull request Jan 31, 2025
…-compatible DataFusion expressions (apache#638)

* convert into workspace project

* update GitHub actions

* update Makefile

* fix regression

* update target path

* update protobuf path in pom.xml

* update more paths

* add new datafusion-comet-expr crate

* rename CometAbsFunc to Abs and add documentation

* fix error message

* improve error handling

* update crate description

* remove unused dep

* address feedback

* finish renaming crate

* update README for datafusion-spark-expr

* rename crate to datafusion-comet-spark-expr
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
…-compatible DataFusion expressions (apache#638)

* convert into workspace project

* update GitHub actions

* update Makefile

* fix regression

* update target path

* update protobuf path in pom.xml

* update more paths

* add new datafusion-comet-expr crate

* rename CometAbsFunc to Abs and add documentation

* fix error message

* improve error handling

* update crate description

* remove unused dep

* address feedback

* finish renaming crate

* update README for datafusion-spark-expr

* rename crate to datafusion-comet-spark-expr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants