-
Notifications
You must be signed in to change notification settings - Fork 215
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
#1047 Expose AddFunction API for CESQL Parser #1051
base: main
Are you sure you want to change the base?
#1047 Expose AddFunction API for CESQL Parser #1051
Conversation
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.
Hey @dgeorgievski at a first look, these changes seem good! My only thought is to maybe not use the tck tests for this, as those tests are meant to test the CESQL spec and are mirrored from the cloudevents spec repo. Since the spec only says An engine SHOULD also allow users to define their custom functions
, I think it is better in our case to just have the test cases you added be in their own test file somewhere else.
I'll take a closer look at the implementation next Monday!
I agree. Let's decide first on the new location of the tests. My first impulse was to place them under |
Yeah putting them into |
…v2/runtime Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>
dd245e6
to
31e1907
Compare
Running user defined functions tests
|
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.
Just one small question about where files go, otherwise LGTM from me!
SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package runtime_test |
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.
Since this is in a separate package for import purposes, do you think it would make sense to move it into a new (nested) directory like /runtime/test
(which would also then have all the tck files in the same directory)>
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.
Yes, I believe creating a dedicated test directory would provide more flexibility for future customizations. Will be right back.
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.
Tests assets moved to runtime/test
dir.
Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>
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.
LGTM from me
@@ -0,0 +1,209 @@ | |||
/* | |||
Copyright 2021 The CloudEvents Authors |
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.
should be 2024
- name: HASPREFIX (5) | ||
expression: "HASPREFIX('abcdef', 'abcdefg')" | ||
result: 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.
remove newline?
return tc.Result | ||
} | ||
|
||
func Test_functionTable_AddFunction(t *testing.T) { |
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.
We tend to follow the common go test naming convention: TestPascalCase. TestFunctionTableAddFunction
)
} | ||
} | ||
|
||
func Test_UserFunctions(t *testing.T) { |
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.
same here: TestUserFunctions
As discussed in #1047 added support for addition of user defined functions.
@Cali0707, waiting on your feedback. The PR has a working version of the implementation, but it might need to be refined especially when it comes to testing,
I've noticed that the
test/tck_test.go
manages all the testing of SQL expressions and functions. I've also addedruntime/functions_resolver_test.go
to be able to test the newruntime.AddFunction
function. Is that ok?