Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Window Function support #128

Closed
wants to merge 2 commits into from

Conversation

Xuyuanp
Copy link
Contributor

@Xuyuanp Xuyuanp commented Aug 13, 2019

Add Window Function support.

Build sql like this

goqu.From("test").Select(goqu.ROW_NUMBER().Over(goqu.W().PartitionBy("a").OrderBy("b")))

More examples:

  • func ExampleW inexpressions_example_test.go
  • test cases

@@ -117,6 +133,45 @@ func COALESCE(vals ...interface{}) exp.SQLFunctionExpression {
return exp.NewSQLFunctionExpression("COALESCE", vals...)
}

func ROW_NUMBER() exp.SQLWindowFunctionExpression {

Choose a reason for hiding this comment

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

don't use ALL_CAPS in Go names; use CamelCase (from golint)

return WFunc("RANK")
}

func DENSE_RANK() exp.SQLWindowFunctionExpression {

Choose a reason for hiding this comment

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

don't use ALL_CAPS in Go names; use CamelCase (from golint)

return WFunc("DENSE_RANK")
}

func PERCENT_RANK() exp.SQLWindowFunctionExpression {

Choose a reason for hiding this comment

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

don't use ALL_CAPS in Go names; use CamelCase (from golint)

sql_dialect.go Outdated
}
if withName {
name := we.Name()
if len(name) == 0 {

Choose a reason for hiding this comment

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

emptyStringTest: replace len(name) == 0 with name == "" (from gocritic)

sql_dialect.go Outdated

if sqlWinFunc, ok := sqlFunc.(exp.SQLWindowFunctionExpression); ok {
b.Write(d.dialectOptions.WindowOverFragment)
if sqlWinFunc.HasWindowName() {

Choose a reason for hiding this comment

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

ifElseChain: rewrite if-else to switch statement (from gocritic)

exp/window.go Outdated
orderCols ColumnListExpression
}

func NewWindowExpression(window string, parent string, partitionCols, orderCols ColumnListExpression) WindowExpression {

Choose a reason for hiding this comment

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

paramTypeCombine: func(window string, parent string, partitionCols, orderCols ColumnListExpression) WindowExpression could be replaced with func(window, parent string, partitionCols, orderCols ColumnListExpression) WindowExpression (from gocritic)


opts.SupportsWindowFunction = true
d = sqlDialect{dialect: "test", dialectOptions: opts}
b = sb.NewSQLBuilder(false)

Choose a reason for hiding this comment

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

SA4006: this value of b is never used (from staticcheck)

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #128 into master will increase coverage by 2.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   87.45%   89.73%   +2.27%     
==========================================
  Files          47       49       +2     
  Lines        3556     3780     +224     
==========================================
+ Hits         3110     3392     +282     
+ Misses        386      328      -58     
  Partials       60       60
Impacted Files Coverage Δ
exp/exp.go 22.22% <ø> (ø) ⬆️
exp/window.go 100% <100%> (ø)
sql_dialect_options.go 100% <100%> (+21.63%) ⬆️
exp/col.go 86.36% <100%> (+11.36%) ⬆️
expressions.go 100% <100%> (+4.08%) ⬆️
exp/select_clauses.go 97.8% <100%> (+0.19%) ⬆️
sql_dialect.go 86.95% <100%> (+0.88%) ⬆️
dialect/sqlite3/sqlite3.go 100% <100%> (ø) ⬆️
dialect/mysql/mysql.go 100% <100%> (ø) ⬆️
exp/window_func.go 100% <100%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3237124...5016724. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #128 into master will increase coverage by 1.97%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   89.94%   91.92%   +1.97%     
==========================================
  Files          47       49       +2     
  Lines        3561     3788     +227     
==========================================
+ Hits         3203     3482     +279     
+ Misses        324      272      -52     
  Partials       34       34
Impacted Files Coverage Δ
exp/exp.go 66.66% <ø> (ø) ⬆️
exp/window.go 100% <100%> (ø)
sql_dialect_options.go 100% <100%> (+18.75%) ⬆️
exp/col.go 86.36% <100%> (+11.36%) ⬆️
expressions.go 100% <100%> (+4.08%) ⬆️
exp/select_clauses.go 97.8% <100%> (+0.19%) ⬆️
sql_dialect.go 96.39% <100%> (+0.26%) ⬆️
dialect/sqlite3/sqlite3.go 100% <100%> (ø) ⬆️
dialect/mysql/mysql.go 100% <100%> (ø) ⬆️
exp/window_func.go 100% <100%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37cfb20...6966a18. Read the comment docs.

@Xuyuanp
Copy link
Contributor Author

Xuyuanp commented Aug 15, 2019

I'm not sure if we should use window function in CamelCase.

@doug-martin
Copy link
Owner

@Xuyuanp I think we should stick with the go convetions of camel case.

I have had the same dilemma with other expression names and ultimately decided to go with the golang conventions.

Copy link
Owner

@doug-martin doug-martin left a comment

Choose a reason for hiding this comment

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

Nice addition! Thank you for taking the time to create all of the tests and examples!

func (swfe sqlWindowFunctionExpression) As(val interface{}) AliasedExpression {
return aliased(swfe, val)
}
func (swfe sqlWindowFunctionExpression) Eq(val interface{}) BooleanExpression { return eq(swfe, val) }
Copy link
Owner

Choose a reason for hiding this comment

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

Are all of these valid comparisons for an SQLWindowFunction? If they are can you provide some examples of when you would use them otherwise I think we should remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like

SELECT ROW_NUMBER() OVER () > 1 FROM test

SELECT ROW_NUMBER() OVER () BETWEEN  1 AND 10 FROM test

maybe nobody write sql like this, but it just works.

I copied these lines from sqlFunctionExpression in case they should be needed.

@doug-martin
Copy link
Owner

I'm going to work on getting this up to date with master and released.

doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
doug-martin added a commit that referenced this pull request Aug 25, 2019
* [ADDED] Window Function support #128 - @Xuyuanp
@Xuyuanp Xuyuanp deleted the window_function_support branch August 28, 2019 07:41
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.

None yet

3 participants