Add lua filter #110

arl · 2020-12-09T11:36:21Z

❓ What

Add a LUA filter that allows to add filter logic (implement the Process function) in lua.
Needs further testing/benchmarking/polishing but first results shows a relatively good performance with respect to the same filter coded in Go (5% slower for a filter with a single lua operation).

Every Lua baker filter compiles a given script and will run a user-specific function (which should have the following prototype):

-- rec is a record object
-- next is function next(record)
function myLuaFilterFunction(rec, next)

lua script knows the record type which is the lua-equivalent of the Record Go interface. record has the following methods:

get(int) string
set(int, string)
clear()
copy() record

In the lua context (called lua state), some utility functions are already defined and can be used:

validateRecord(record) bool, int
createRecord record
fieldByName(string) int|nil different from Go which returns (baker.FieldIndex, bool)
fieldNames which is a lua table, same as fieldNames Go slice

🔨 How to test

List all steps necessary;
To test this pull request.

✅ Checklists

This section contains a list of checklists for common uses, please delete the checklists that are useless for your current use case (or add another checklist if your use case isn't covered yet).

Is there unit/integration test coverage for all new and/or changed functionality added in this PR?
Have the changes in this PR been functionally tested?
Have new components been added to the related all.go files?
Have new components been added to the documentation website?
Has make gofmt-write been run on the code?
Has make govet been run on the code? Has the code been fixed accordingly to the output?
Have the changes been added to the CHANGELOG.md file?
Have the steps in CONTRIBUTING.md been followed to update a Go module?

codecov-io · 2020-12-12T19:50:01Z

Codecov Report

Merging #110 (e5da03b) into main (3ff24dd) will increase coverage by 0.79%.
The diff coverage is 82.29%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   49.43%   50.22%   +0.79%     
==========================================
  Files          56       57       +1     
  Lines        3880     3976      +96     
==========================================
+ Hits         1918     1997      +79     
- Misses       1752     1764      +12     
- Partials      210      215       +5

Impacted Files	Coverage Δ
filter/lua.go	`82.29% <82.29%> (ø)`

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 3ff24dd...e5da03b. Read the comment docs.

filter/lua.go

tommyblue · 2021-03-04T14:31:15Z

filter/lua.go

+
+This filter is based on [GopherLua](github.com/yuin/gopher-lua), which is an LUA5.1 virtual machine.
+
+To use this filter you need to declare a function in an lua file. This function serves the same purpose


maybe it's worth telling the user that the function name must be the same as FilterName conf

I'm not sure I see how can the user could be confused here since the config h as 2 fields: a filename and a function name.

filter/lua.go

lucarin91 · 2021-03-04T15:05:07Z

filter/lua.go

+func (t *LUA) Process(rec baker.Record, next func(baker.Record)) {
+	atomic.AddInt64(&t.nprocessed, 1)
+
+	t.mu.Lock()


Are you sure to lock all the Process functions? This means that even if the Lua function is pure it will not exploits any concurrent execution on different disjoined records. Moreover, it will imply that adding a single LuaFilter on a possibly long and complex filter-chain will inhibit any parallel execution, blocking all the execution on a single lock.
IHMO we need to create a new state each time to avoid the lock.

unlocking before next would solve the filterchain parallelization issue, no?

I think we should refocus the review on correctness and semantics of the user-api.
Creating a new state each time is probably quite expensive, it might be beneficial in some cases, in might not be in others and I'm not sure one can be certain it's the way to go to obtain the best overall performance in a majority of cases.

Happy to discuss optimizations later once we get the API right though

unlocking before next would solve the filterchain parallelization issue, no?

the user may not call next so I'm not sure I understand

Since the lua function is in a file, the user could have written some statements before the function. This statement may be needed for the filter to work for example. The current implementation allows that by creating the state once and locking it each time any goroutine enters the lua filter. Recreating the state each time, apart from the potential cost, removes this possibility, since the lua file would need to be reinterpreted each time, that means for each record, at least if I correctly understood what you meant.

Another possible API I thought of was to ask the user to code the lua function directly in the TOML, but you lose syntax highlighting when coding in lua, you'll need to escape your code, etc.

I think that it will be better to ask the user to write pure functions, actually, I will see strange the other way around. Moreover, we could use this approach, sharing-lua-byte-code-between-lstates, to compile one time the Lua file and use it with different states. Although, it is thought that calling NewState each time seems a bit too costly.

Trying to refocus the discussion a bit.

The idea behind the lua filter is to lower the bar in using baker and coding some filters fast. If I have to code pure lua functions, that means I won't be able to do simple stuff one could expect to be able to do in a filter, such as counting records for example.

Apart from being faster (which, as always with performance, would needs to be proved with numbers) I'm not sure I see any other benefit in requiring the user to write pure functions.

I think that we have to clarify, before going into the implementation details, how we want to execute the LUA code of the user. I see two possible alternatives:

sequentially execute the LUA code on the records

parallelize the LUA code on multiple records

The two alternatives have pros and cons and both could drastically change both the performance and the expressiveness of the user. We could also think of a configurable behavior of the filter, but in both cases, the user must be aware of what he/she can and cannot do.

I agree in any case the user must know the tradeoffs that have been made so they can chose if they match their use case so those have to be documented.

Coming back to @tommyblue comments about the position of unlock, it seems right to release the lock before calling next to allow concurrent execution of the rest of the filter chain. Will fix this and perform the benchmark/tests with this 👍

Any way to offload the locking to the user inside the Lua code? This way they can indicate if they need synchronization or not.

Suppress one allocation, reducing their number from 5 to 4, by pre-allocating the userdata we use to wrap the record passed to the lua filter. name old time/op new time/op delta LUAProcess-8 638ns ± 4% 559ns ± 6% -12.31% (p=0.000 n=10+10) name old alloc/op new alloc/op delta LUAProcess-8 152B ± 0% 104B ± 0% -31.58% (p=0.000 n=10+10) name old allocs/op new allocs/op delta LUAProcess-8 5.00 ± 0% 4.00 ± 0% -20.00% (p=0.000 n=10+10)

Reduce allocation from 4 to 2 by preallocating the lua function which is a wrapper to the Go 'next' function we receive. name old time/op new time/op delta LUAProcess-8 559ns ± 6% 459ns ± 7% -17.97% (p=0.000 n=10+10) name old alloc/op new alloc/op delta LUAProcess-8 104B ± 0% 24B ± 0% -76.92% (p=0.000 n=10+10) name old allocs/op new allocs/op delta LUAProcess-8 4.00 ± 0% 2.00 ± 0% -50.00% (p=0.000 n=10+10)

Lua function just receives a lua Record, and must return 2 values: - the first is a boolean indicating whether is to be kept or discarded - the second can be nil or a string indicating the error for this record

codecov-commenter · 2021-11-15T18:43:21Z

Codecov Report

Merging #110 (68a1060) into main (40a57bc) will decrease coverage by 3.75%.
The diff coverage is 81.25%.

❗ Current head 68a1060 differs from pull request most recent head 4fe5623. Consider uploading reports for the commit 4fe5623 to get more accurate results

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
- Coverage   53.95%   50.20%   -3.76%     
==========================================
  Files          60       57       -3     
  Lines        4003     3976      -27     
==========================================
- Hits         2160     1996     -164     
- Misses       1646     1764     +118     
- Partials      197      216      +19

Impacted Files	Coverage Δ
filter/lua.go	`81.25% <81.25%> (ø)`
filter/set_string_from_url.go	`37.50% <0.00%> (-19.03%)`	⬇️
topology.go	`60.89% <0.00%> (-13.41%)`	⬇️
config.go	`65.00% <0.00%> (-13.00%)`	⬇️
filter/metadata_lastmodified.go	`76.19% <0.00%> (-12.70%)`	⬇️
input/list.go	`60.78% <0.00%> (-10.51%)`	⬇️
filter/notnull.go	`70.58% <0.00%> (-9.42%)`	⬇️
filter/clear_fields.go	`56.25% <0.00%> (-8.04%)`	⬇️
filter/partial_clone.go	`65.00% <0.00%> (-7.23%)`	⬇️
filter/string_match.go	`69.56% <0.00%> (-6.63%)`	⬇️
... and 26 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 a209176...4fe5623. Read the comment docs.

arl force-pushed the add-lua-filter branch 2 times, most recently from 81400af to 8b1247f Compare December 12, 2020 12:15

arl changed the base branch from main to fieldname-as-slice December 13, 2020 21:11

arl force-pushed the add-lua-filter branch from fe96468 to f048c34 Compare December 13, 2020 21:34

arl force-pushed the add-lua-filter branch from 38f2b64 to 1b2c8e5 Compare December 26, 2020 20:35

arl marked this pull request as ready for review December 27, 2020 10:24

arl force-pushed the fieldname-as-slice branch 2 times, most recently from 7c42f57 to f22308b Compare January 7, 2021 11:51

arl force-pushed the add-lua-filter branch from 1b2c8e5 to 87dfc44 Compare January 7, 2021 11:53

Base automatically changed from fieldname-as-slice to main January 7, 2021 17:38

arl force-pushed the add-lua-filter branch from 87dfc44 to 91387cf Compare January 7, 2021 17:41

arl force-pushed the add-lua-filter branch from 888f532 to 1e06b82 Compare February 10, 2021 10:31

arl force-pushed the add-lua-filter branch from 1e06b82 to e3c853d Compare March 3, 2021 18:37

arl requested a review from tommyblue March 3, 2021 18:37

arl force-pushed the add-lua-filter branch from 918ba3c to 53e92a9 Compare March 3, 2021 18:50

lucarin91 suggested changes Mar 4, 2021

View reviewed changes

filter/lua.go Outdated Show resolved Hide resolved

filter/lua.go Outdated Show resolved Hide resolved

filter/lua.go Outdated Show resolved Hide resolved

filter/lua.go Outdated Show resolved Hide resolved

filter/lua.go Show resolved Hide resolved

lucarin91 reviewed Mar 4, 2021

View reviewed changes

filter/lua.go Outdated Show resolved Hide resolved

tommyblue reviewed Mar 4, 2021

View reviewed changes

lucarin91 reviewed Mar 4, 2021

View reviewed changes

arl added 10 commits November 15, 2021 19:42

filter: lua: initial commit

f2d1a09

filter: lua: fastcheckLuaRecord

06484d8

filter: lua: bind fields, createRecord and validateRecord

edbff9c

filter: lua: implement lua record copy+clear

065649d

filter: lua: check lua filter function exists

7024954

filter: lua: add help strings

7a2fa43

filter: lua: cosmetics + docs

fff2018

filter: lua: fieldName and fieldByName

b9d204f

arl added 10 commits November 15, 2021 19:42

Update CHANGELOG.md

c637d75

filter: lua: cosmetics

0117102

filter: lua: improve filter documentation

f6bd729

filter: lua: remove limit on procs=1

3e54ab7

output: lua: implement Stats() method

19e4286

Cosmetics

7b3fd95

filter: lua: address cosmetic review comments

73224ef

filter: remove NumProcessedLines

ab5f287

bump lua

1ebd394

filter: lua: simplify process api

4fe5623

Lua function just receives a lua Record, and must return 2 values: - the first is a boolean indicating whether is to be kept or discarded - the second can be nil or a string indicating the error for this record

arl force-pushed the add-lua-filter branch from cfd9d12 to 4fe5623 Compare November 15, 2021 18:42

filter: lua: fix tests

341f08b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lua filter #110

Add lua filter #110

arl commented Dec 9, 2020 •

edited

Loading

codecov-io commented Dec 12, 2020 •

edited

Loading

tommyblue Mar 4, 2021

arl Mar 4, 2021

lucarin91 Mar 4, 2021

tommyblue Mar 4, 2021

arl Mar 4, 2021

arl Mar 4, 2021

arl Mar 4, 2021

lucarin91 Mar 4, 2021

arl Mar 4, 2021

lucarin91 Mar 5, 2021

arl Mar 5, 2021 •

edited

Loading

Oppen Jul 28, 2021

codecov-commenter commented Nov 15, 2021


		This filter is based on [GopherLua](github.com/yuin/gopher-lua), which is an LUA5.1 virtual machine.

		To use this filter you need to declare a function in an lua file. This function serves the same purpose

Add lua filter #110

Are you sure you want to change the base?

Add lua filter #110

Conversation

arl commented Dec 9, 2020 • edited Loading

❓ What

🔨 How to test

✅ Checklists

codecov-io commented Dec 12, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arl Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 15, 2021

Codecov Report

arl commented Dec 9, 2020 •

edited

Loading

codecov-io commented Dec 12, 2020 •

edited

Loading

arl Mar 5, 2021 •

edited

Loading