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
Limit the subquery results by memory usage #13952
Limit the subquery results by memory usage #13952
Changes from 37 commits
3e4935a
04bb5b8
e7f0f9f
b96a671
6b119a3
36e1f37
633f83a
9cc62d2
f3db103
038b3b5
96ad351
74f06cd
2866de3
eda4c42
8a07da1
fd32127
ea56f4f
15b49b0
8e44ec6
b834f8e
42d0698
9bc6b3a
fe5d203
ac5cc02
37e217b
1f17557
7e2a052
f4b87d1
38a28c0
0db119a
663a62d
bf39600
bdab41a
1ec37af
6acf97a
4531e4d
50cda6a
7284729
cc99705
7243ce3
d432dfe
4e7a540
27a8566
6a45ea9
8b3ab89
1d110b8
bc346c9
235fdb3
755a41d
0f60d97
0feebee
7f62179
97a8197
92c3b52
4d7b3c4
dd3e140
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.
IMO adding
allowNullColumnTypes
as a parameter here isn't the right approach. Better for the caller to replace all the unknown types (null
) insignature
withCOMPLEX<json>
, if that's what's desired. It keeps the frame writer code simpler, easier to test/understand, etc.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.
Use
@Nullable
where appropriate, but avoid@Nonnull
on parameters in general, since it creates an implication that unannotated things can be null. But, they mostly can't, so it's misleading.If you're looking for benefit for static analyzers, better to use
@EverythingIsNonnullByDefault
at package level throughpackage-info.java
. We have that on various packages already and could add it to more as desired. That's like annotating every un-annotated parameter with@Nonnull
, without the clutter.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 for the pointer! Will fix the clutter.
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.
This method seems weird. You can call the base method directly.
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.
Added this as a separate method originally because we don't require the boolean in the rest of the cases (i.e. the original ones in MSQ). Therefore it made sense to me to hide this complexity from the callers of this method that arent residing in the broker.
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.
Can you let us know the reason why do you think adding the serializer here makes sense.
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 would think we don't need to serialize this, as it should exist in-memory only. So I'm also wondering where this was needed.
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.
This serialization is required when the broker inlines the subquery results and sends the inlined query to the historicals. In that case, we serialize the Frames and FrameBasedInlineDatasource to behave as if an equivalent InlineDatasource (based on rows) would have been serialized.