Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/230 get partitioning checkpoints #265
Feature/230 get partitioning checkpoints #265
Changes from 20 commits
6b2df24
4664180
099ce5a
2bd1548
e4cf419
3e69c7f
d59d7da
eb01623
1b28b7c
4a5e925
67cab0b
dfc8ad9
1823dfc
d3d98c8
9716301
0f15e1c
2bf339e
dd3b7e2
71c1c33
611ea75
7811f37
c33a136
4e0f6ba
e187745
92a2099
1e14e10
4b2a9db
8811b4e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Alternate solution, no need to change the current one, just for inspiration:
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.
Complicated
First I thought this was a mistake to have
ORDER BY
omitted from the query.Then I thought: wait we don't care about the order of we just want to see if there are more records.
Then I thought: but having the
ORDER BY
will cause to have the same data pages cached in memory for the query bellow.And then I thought: but the ordering might be more expensive then the gain the cache gives.
So I don't know 🤣
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.
let's keep the code as is then for now :)
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.
Personal quirk, no need to change here:
I do prefer
INNER JOIN
toJOIN
to make it obvious. So if in future you would consider using that 👼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.
ok, will add the 'INNER' and do so going forward always
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.
Major:
We want the measurements of one checkpoint go after each other followed by the next checkpoint etc.
While the current way how the measurements come in (all at once) ensure this gives the correct order, this is not guaranteed by the data itself, just by actual processes right now.
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.
ok switched the order
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.
Is this a problem?
I think it's valid situation, if no checkpoint for an existing partitioning exist so far.
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.
Actually, so far in queries returning detail records of a master one, of there were no details one, we ust returned the empty set. Would prefer we stay consistent, and if for some reason this is considered better, changer there too.
Tag @TebaleloS @lsulak
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 you're right. How about status 12 with text 'OK with no checkpoints found'?
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 OK solution might cause even more problems, as the data returned are NULL. So fa-db would need to be able to handle that and recognize that the particular record is OK but return empty sequence.
In the other cases we just returned no records at all.