Skip to content

implement range/generate_series func - #8140

Merged
alamb merged 7 commits into
apache:mainfrom
Veeupup:func_range
Nov 14, 2023
Merged

implement range/generate_series func#8140
alamb merged 7 commits into
apache:mainfrom
Veeupup:func_range

Conversation

@Veeupup

@VeeupupVeeupup commented Nov 11, 2023

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#8028 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

test in sqllogictests

Are there any user-facing changes?

@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Nov 11, 2023
@Veeupup

Copy link
Copy Markdown
ContributorAuthor

cc @jayzhan211@alamb this PR is ready for review! : )

@2010YOUY01

Copy link
Copy Markdown
Contributor

Thank you for this new function! Looks like these functions are available in DuckDB 👍🏼 https://duckdb.org/docs/sql/functions/nested.html#range-functions

Here are some suggestions:

  1. Can we add more corner case tests like range(5,5,0), range(1,5,-1), range(1,-5,1)...
  2. Include the semantics of this function (is start/end index inclusive or exclusive, what will happen for range(1,5,-1)) in SQL doc: https://github.com/apache/arrow-datafusion/blob/main/docs/source/user-guide/sql/scalar_functions.md

@jayzhan211jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

Comment threaddatafusion/expr/src/expr_fn.rs
Comment threaddatafusion/expr/src/expr_fn.rs
@Veeupup

Copy link
Copy Markdown
ContributorAuthor

Thank you for this new function! Looks like these functions are available in DuckDB 👍🏼 https://duckdb.org/docs/sql/functions/nested.html#range-functions

Here are some suggestions:

  1. Can we add more corner case tests like range(5,5,0), range(1,5,-1), range(1,-5,1)...
  2. Include the semantics of this function (is start/end index inclusive or exclusive, what will happen for range(1,5,-1)) in SQL doc: https://github.com/apache/arrow-datafusion/blob/main/docs/source/user-guide/sql/scalar_functions.md

thanks! I have made it clear in the test and doc.

@2010YOUY012010YOUY01 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the doc. I think the panic should be addressed. Everything else looks good to me

Comment threaddocs/source/user-guide/sql/scalar_functions.md
Comment threaddatafusion/physical-expr/src/array_expressions.rs

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @Veeupup -- this looks great. I took the liberty of merging this branch up from main to resolve a conflict.

It would be great if you could add a doc comment explaining the arguments to gen_range , but I also think we could do that as a follow on PR if necessary.

Thanks again for the recent string of nice PRs 🙏

Comment threaddatafusion/physical-expr/src/array_expressions.rs
Signed-off-by: veeupup <code@tanweime.com>
@alamb

Copy link
Copy Markdown
Contributor

Thanks again @Veeupup and @jayzhan211

@alamb
alamb merged commit 31e54f0 into apache:mainNov 14, 2023
@Veeupup
Veeupup deleted the func_range branch November 21, 2023 15:07
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-exprLogical plan and expressionsphysical-exprChanges to the physical-expr cratessqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support udf range

4 participants

@Veeupup@2010YOUY01@alamb@jayzhan211