-
Notifications
You must be signed in to change notification settings - Fork 360
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
Test: Lua Hive Partition pager #6691
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.
Thanks, neat and useful stuff! Only minor changes requested, but they are blocking.
local hive = require("lakefs/catalogexport/hive") | ||
|
||
-- helper function to slice table array | ||
function table.slice(tbl, first, last, step) |
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.
AFAICT this implementation doesn't really slice a table array. For instance if the table is sparse, it is still an array -- but its table_slice
will be a dense array "table".
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.
changed name
local hive = require("lakefs/catalogexport/hive") | ||
|
||
-- helper function to slice table array | ||
function table.slice(tbl, first, last, step) |
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.
"table.slice" conflicts with standard Lua table methods. Perhaps pretend that these live in "xtable"?
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 think the link is wrong and I couldn't find any reference to what you're saying, can you link the source that shows it's a conflict with standard lua methods?
Either way, I didn't like that fact that it's global table it's just a helper function for very specific use case that can't be shared outside this code anyway, so I changed the name and the scope.
end | ||
} | ||
|
||
local page = 2 |
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.
local page = 2 | |
local page_size = 2 |
Not sure why we're using a different name than the parameter.
Also: can we check with multiple page_size
arguments? I would gladly check every page size from 1 to the entire size of the list, or even the entire size of the list plus 2. Each should give the same output.
Otherwise some pager bugs are just too easy to miss.
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.
Both points done, thanks!
Thanks for the review @arielshaqed ! Please look again. |
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.
Yay, thanks!
Closes #6720
This is a sunny day test to check that partitions pager works as expected.