-
Notifications
You must be signed in to change notification settings - Fork 15
Refactor select plan #32
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
Conversation
Plan is generated on router and is passed to storage Filter functions are generated on storage
| end | ||
|
|
||
| -- set after tuple | ||
| local after_tuple = utils.flatten(opts.after, space_format) |
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.
Not sure it's a good place for unflatten...
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.
Why?
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.
I believe we should pass raw tuple to this function as option...
But ok, feel free to ignore this comment
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 crud returns map, user will use it as an after value for selecting next batch of data.
crud/select/executor.lua
Outdated
| function executor.execute(space, index, scan_value, iter, filter_func, opts) | ||
| checks('table', 'table', '?table', 'number', 'function', { | ||
| after_tuple = '?cdata|table', | ||
| batch_size = '?number', |
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.
number|cdata
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.
Or even number|uint64|int64
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.
So, not sure checks really needed here it's internal module
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, but it's very helpful for local development.
Moreover, all functions have a lot of arguments and different opts.
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.
very helpful for local development.
Then do something like:
local checks = require('checks')
if os.getenv('DEV') ~= '1' then
checks = function() end
endAnd run all your tests with "DEV=1" option.
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.
See #35.
crud/common/utils.lua
Outdated
| return merged_parts | ||
| end | ||
|
|
||
| function utils.extract_sharding_key_from_scan_key(scan_key, scan_index, sharding_index) |
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.
It does the same as is_scan_by_full_sharding_key_eq
I didn't forget about
Closes #30