Skip to content

DataFrame supports window function - #1167

Merged
jimexist merged 1 commit into
apache:masterfrom
xudong963:df_add_window
Oct 26, 2021
Merged

DataFrame supports window function#1167
jimexist merged 1 commit into
apache:masterfrom
xudong963:df_add_window

Conversation

@xudong963

@xudong963xudong963 commented Oct 22, 2021

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes#1147

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@houqp
houqp requested a review from jimexistOctober 23, 2021 06:54
@houqphouqp added the enhancement New feature or request label Oct 23, 2021
jimexist
jimexist approved these changes Oct 23, 2021
View reviewed changes

@jimexistjimexist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for working on this!

@jimexist

jimexist commented Oct 24, 2021

Copy link
Copy Markdown
Member

it might be worthwhile to note down the code duplication here - i did plan to optimize for reordering of sorts in future if multiple window functions exist.

houqp
houqp approved these changes Oct 25, 2021
View reviewed changes

@houqphouqp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree with what @jimexist said, I think logical plan builder is probably the best place to host the duplicated code, for example: LogicalPlanBuilder::from(plan)::windows(window_func_exprs), which takes a list of window expressions, then calls self.window over them.

@github-actionsgithub-actionsBot added the sql SQL Planner label Oct 25, 2021
@xudong963

xudong963 commented Oct 25, 2021

Copy link
Copy Markdown
MemberAuthor

Thanks for your comments, I encapsulated redundant code in LogicalPlanBuilder. PTAL @jimexist@houqp

houqp
houqp approved these changes Oct 25, 2021
View reviewed changes

@houqphouqp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks @xudong963. I will let @jimexist do the final review and merge :)

Comment threaddatafusion/src/sql/planner.rs Outdated
@@ -840,24 +839,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

/// Wrap a plan in a window
fn window(&self, input: LogicalPlan, window_exprs: Vec<Expr>) -> Result<LogicalPlan> {

@houqphouqpOct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

perhaps we could get rid of this function as well since it now just does a single function call :)

@xudong963xudong963Oct 26, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

nice caught!

jimexist
jimexist reviewed Oct 26, 2021
View reviewed changes
Comment threaddatafusion/src/logical_plan/builder.rs Outdated
}

/// Wrap a plan in a window
pub fn window_plan(

@jimexistjimexistOct 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can this be crate public?

jimexist
jimexist approved these changes Oct 26, 2021
View reviewed changes

@jimexistjimexist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm thanks

@jimexist
jimexist merged commit b9ef8de into apache:masterOct 26, 2021
@jimexistjimexist mentioned this pull request Oct 26, 2021
Merged
@xudong963
xudong963 deleted the df_add_window branch October 26, 2021 03:19
jgoday pushed a commit to jgoday/arrow-datafusion that referenced this pull request Oct 27, 2021
@xudong963@jgoday
DataFrame supports window function (apache#1167)
0057285
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
@andygrove
chore: Add ignored tests for reading complex types from Parquet (apac…
f1d0879
…he#1167)
* Add ignored tests for reading structs from Parquet
* add basic map test
* add tests for Map and Array
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
@dharanad@andygrove
@kazuyukitanimura@parthchandra@viirya@rluvaton@alamb
feat: add support for array_contains expression (apache#1163)
4f8ce75
* feat: add support for array_contains expression
* test: add unit test for array_contains function
* Removes unnecessary case expression for handling null values
* chore: Move more expressions from core crate to spark-expr crate (apache#1152)
* move aggregate expressions to spark-expr crate
* move more expressions
* move benchmark
* normalize_nan
* bitwise not
* comet scalar funcs
* update bench imports
* remove dead code (apache#1155)
* fix: Spark 4.0-preview1 SPARK-47120 (apache#1156)
## Which issue does this PR close?
Part of apache/datafusion-comet#372 and apache/datafusion-comet#551
## Rationale for this change
To be ready for Spark 4.0
## What changes are included in this PR?
This PR fixes the new test SPARK-47120 added in Spark 4.0
## How are these changes tested?
tests enabled
* chore: Move string kernels and expressions to spark-expr crate (apache#1164)
* Move string kernels and expressions to spark-expr crate
* remove unused hash kernel
* remove unused dependencies
* chore: Move remaining expressions to spark-expr crate + some minor refactoring (apache#1165)
* move CheckOverflow to spark-expr crate
* move NegativeExpr to spark-expr crate
* move UnboundColumn to spark-expr crate
* move ExpandExec from execution::datafusion::operators to execution::operators
* refactoring to remove datafusion subpackage
* update imports in benches
* fix
* fix
* chore: Add ignored tests for reading complex types from Parquet (apache#1167)
* Add ignored tests for reading structs from Parquet
* add basic map test
* add tests for Map and Array
* feat: Add Spark-compatible implementation of SchemaAdapterFactory (apache#1169)
* Add Spark-compatible SchemaAdapterFactory implementation
* remove prototype code
* fix
* refactor
* implement more cast logic
* implement more cast logic
* add basic test
* improve test
* cleanup
* fmt
* add support for casting unsigned int to signed int
* clippy
* address feedback
* fix test
* fix: Document enabling comet explain plan usage in Spark (4.0) (apache#1176)
* test: enabling Spark tests with offHeap requirement (apache#1177)
## Which issue does this PR close?
## Rationale for this change
After apache/datafusion-comet#1062 We have not running Spark tests for native execution
## What changes are included in this PR?
Removed the off heap requirement for testing
## How are these changes tested?
Bringing back Spark tests for native execution
* feat: Improve shuffle metrics (second attempt) (apache#1175)
* improve shuffle metrics
* docs
* more metrics
* refactor
* address feedback
* fix: stddev_pop should not directly return 0.0 when count is 1.0 (apache#1184)
* add test
* fix
* fix
* fix
* feat: Make native shuffle compression configurable and respect `spark.shuffle.compress` (apache#1185)
* Make shuffle compression codec and level configurable
* remove lz4 references
* docs
* update comment
* clippy
* fix benches
* clippy
* clippy
* disable test for miri
* remove lz4 reference from proto
* minor: move shuffle classes from common to spark (apache#1193)
* minor: refactor decodeBatches to make private in broadcast exchange (apache#1195)
* minor: refactor prepare_output so that it does not require an ExecutionContext (apache#1194)
* fix: fix missing explanation for then branch in case when (apache#1200)
* minor: remove unused source files (apache#1202)
* chore: Upgrade to DataFusion 44.0.0-rc2 (apache#1154)
* move aggregate expressions to spark-expr crate
* move more expressions
* move benchmark
* normalize_nan
* bitwise not
* comet scalar funcs
* update bench imports
* save
* save
* save
* remove unused imports
* clippy
* implement more hashers
* implement Hash and PartialEq
* implement Hash and PartialEq
* implement Hash and PartialEq
* benches
* fix ScalarUDFImpl.return_type failure
* exclude test from miri
* ignore correct test
* ignore another test
* remove miri checks
* use return_type_from_exprs
* Revert "use return_type_from_exprs"
This reverts commit febc1f1ec1301f9b359fc23ad6a117224fce35b7.
* use DF main branch
* hacky workaround for regression in ScalarUDFImpl.return_type
* fix repo url
* pin to revision
* bump to latest rev
* bump to latest DF rev
* bump DF to rev 9f530dd
* add Cargo.lock
* bump DF version
* no default features
* Revert "remove miri checks"
This reverts commit 4638fe3aa5501966cd5d8b53acf26c698b10b3c9.
* Update pin to DataFusion e99e02b
* update pin
* Update Cargo.toml
Bump to 44.0.0-rc2
* update cargo lock
* revert miri change
---------
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* update UT
Signed-off-by: Dharan Aditya <dharan.aditya@gmail.com>
* fix typo in UT
Signed-off-by: Dharan Aditya <dharan.aditya@gmail.com>
---------
Signed-off-by: Dharan Aditya <dharan.aditya@gmail.com>
Co-authored-by: Andy Grove <agrove@apache.org>
Co-authored-by: KAZUYUKI TANIMURA <ktanimura@apple.com>
Co-authored-by: Parth Chandra <parthc@apache.org>
Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Co-authored-by: Raz Luvaton <16746759+rluvaton@users.noreply.github.com>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
HairstonE pushed a commit to HairstonE/datafusion that referenced this pull request Oct 7, 2025
@timsaucer
feat: collect once during display() in jupyter notebooks (apache#1167)
9545634
* Only collect one time during display() in jupyter notebooks
* Check for juypter notebook environment specifically
* Remove approach of checking environment which could not differentiate between jupyter console and notebook
* Instead of trying to detect notebook vs console, collect one time when we have any kind if ipython environment.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimexistjimexistjimexist approved these changes
@houqphouqphouqp approved these changes

Assignees

No one assigned

Labels

enhancementNew feature or requestsqlSQL Planner

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

WindowFunction works with SQL but not with DataFrame

3 participants

@xudong963@jimexist@houqp