-
Notifications
You must be signed in to change notification settings - Fork 213
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
Conversation
@@ -117,6 +133,45 @@ func COALESCE(vals ...interface{}) exp.SQLFunctionExpression { | |||
return exp.NewSQLFunctionExpression("COALESCE", vals...) | |||
} | |||
|
|||
func ROW_NUMBER() exp.SQLWindowFunctionExpression { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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
)
sql_dialect_test.go
Outdated
|
||
opts.SupportsWindowFunction = true | ||
d = sqlDialect{dialect: "test", dialectOptions: opts} | ||
b = sb.NewSQLBuilder(false) |
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
I'm not sure if we should use window function in CamelCase. |
@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. |
There was a problem hiding this 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) } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
5016724
to
6966a18
Compare
I'm going to work on getting this up to date with master and released. |
Add Window Function support.
Build sql like this
More examples:
ExampleW
inexpressions_example_test.go