#1047 Expose AddFunction API for CESQL Parser #1051

dgeorgievski · 2024-05-03T16:52:16Z

As discussed in #1047 added support for addition of user defined functions.

AddFunction API
TCK tests for user defined functions.
Instructions on using user defined functions in README file.

@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 added runtime/functions_resolver_test.go to be able to test the new runtime.AddFunction function. Is that ok?

Cali0707

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!

dgeorgievski · 2024-05-03T20:24:57Z

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 agree. Let's decide first on the new location of the tests. My first impulse was to place them under runtime/ considering I am testing runtime.AddFunction in the same dir as well.

Cali0707 · 2024-05-03T23:45:56Z

Yeah putting them into runtime sounds reasonable to me!

- Initial implementation of a user defined function. - TCK tests for user defined functions. - Instructions on using user defined functions in README file. Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

…v2/runtime Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

dgeorgievski · 2024-05-09T18:25:59Z

Moved user defined functions tests from sql/v2/test to sql/v2/runtime
Created runtime_test package to avoid circular import runtime package errors.
Using the same TCK mechanism for running user defined functions test to simplify the users validation of new functions. For example, if I'd like to test a new function I plan to use in my app, I'd be able to quickly validate it by adding both the function definition and all the required tests cases in these two files without the overhead of creating the required events for testing.
1. runtime/user_defined_functions_test.go.
2. runtime/tck/user_defined_functions.yaml`.

Running user defined functions tests

$ go test -v ./runtime
...
--- PASS: Test_functionTable_AddFunction (0.00s)
    --- PASS: Test_functionTable_AddFunction/Add_user_functions_to_global_table (0.00s)
    --- PASS: Test_functionTable_AddFunction/Fail_add_user_functions_to_global_table (0.00s)
...
--- PASS: Test_UserFunctions (0.01s)
    --- PASS: Test_UserFunctions/User_defined_functions (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/KONKAT_(2) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/KONKAT_(3) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/HASPREFIX_(1) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/HASPREFIX_(3) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/HASPREFIX_(5) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/HASPREFIX_(4) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/HASPREFIX_(2) (0.00s)
        --- PASS: Test_UserFunctions/User_defined_functions/KONKAT_(1) (0.00s)
PASS
ok      github.com/cloudevents/sdk-go/sql/v2/runtime    0.022s

Cali0707

Just one small question about where files go, otherwise LGTM from me!

Cali0707 · 2024-05-09T19:39:14Z

sql/v2/runtime/user_defined_functions_test.go

+ SPDX-License-Identifier: Apache-2.0
+*/
+
+package runtime_test


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)>

Yes, I believe creating a dedicated test directory would provide more flexibility for future customizations. Will be right back.

Tests assets moved to runtime/test dir.

Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

Cali0707

LGTM from me

sql/v2/runtime/test/user_defined_functions_test.go

lionelvillard · 2024-06-04T20:02:07Z

sql/v2/runtime/test/tck/user_defined_functions.yaml

+  - name: HASPREFIX (5)
+    expression: "HASPREFIX('abcdef', 'abcdefg')"
+    result: false
+


remove newline?

The existing sql tck yaml files use new line to separate tests cases.
https://github.com/cloudevents/sdk-go/blob/main/sql/v2/test/tck/string_builtin_functions.yaml#L12-L18

I was following their format. I think the new line between consecutive tests cases should be kept but the new line at the end of the file should be removed.

sql/v2/runtime/test/user_defined_functions_test.go

Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

dgeorgievski requested a review from a team as a code owner May 3, 2024 16:52

Cali0707 reviewed May 3, 2024

View reviewed changes

dgeorgievski added 2 commits May 9, 2024 18:16

Added:

01cc71b

- Initial implementation of a user defined function. - TCK tests for user defined functions. - Instructions on using user defined functions in README file. Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

Moved tests assets for user defined fuctions from sql/v2/test to sql/…

31e1907

…v2/runtime Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

dgeorgievski force-pushed the 1047-expose-addfunction-api-cesql branch from dd245e6 to 31e1907 Compare May 9, 2024 18:17

Cali0707 reviewed May 9, 2024

View reviewed changes

moved runtime test to runtime/test dir

f811b66

Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

Cali0707 approved these changes May 13, 2024

View reviewed changes

lionelvillard reviewed Jun 4, 2024

View reviewed changes

Corrected test functions naming format and copyright year

22596a9

Signed-off-by: Dimitar Georgievski <dgeorgievski@gmail.com>

dgeorgievski force-pushed the 1047-expose-addfunction-api-cesql branch from 2c125d7 to 22596a9 Compare June 5, 2024 15:07

Merge branch 'main' into 1047-expose-addfunction-api-cesql

b877ab4

lionelvillard approved these changes Jun 11, 2024

View reviewed changes

lionelvillard merged commit 4a007a1 into cloudevents:main Jun 11, 2024
8 checks passed

dgeorgievski deleted the 1047-expose-addfunction-api-cesql branch June 11, 2024 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1047 Expose AddFunction API for CESQL Parser #1051

#1047 Expose AddFunction API for CESQL Parser #1051

dgeorgievski commented May 3, 2024

Cali0707 left a comment

dgeorgievski commented May 3, 2024 •

edited

Loading

Cali0707 commented May 3, 2024

dgeorgievski commented May 9, 2024 •

edited

Loading

Cali0707 left a comment

Cali0707 May 9, 2024

dgeorgievski May 9, 2024

dgeorgievski May 9, 2024

Cali0707 left a comment

lionelvillard Jun 4, 2024

dgeorgievski Jun 5, 2024

#1047 Expose AddFunction API for CESQL Parser #1051

#1047 Expose AddFunction API for CESQL Parser #1051

Conversation

dgeorgievski commented May 3, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

dgeorgievski commented May 3, 2024 • edited Loading

Cali0707 commented May 3, 2024

dgeorgievski commented May 9, 2024 • edited Loading

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 May 9, 2024

Choose a reason for hiding this comment

dgeorgievski May 9, 2024

Choose a reason for hiding this comment

dgeorgievski May 9, 2024

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

lionelvillard Jun 4, 2024

Choose a reason for hiding this comment

dgeorgievski Jun 5, 2024

Choose a reason for hiding this comment

dgeorgievski commented May 3, 2024 •

edited

Loading

dgeorgievski commented May 9, 2024 •

edited

Loading