Fix dictionarySize overrides in tests#15354
Conversation
|
the way the code disallowed override made it also loose the custom settings used in |
There was a problem hiding this comment.
Is there an easy way to test out the NullableRowBasedKeySerdeHelper using a query, where the delegate returns false, but, the helper returns true which causes the query to fail with an unexpected message
Would setting a small value of maxDictionarySize trigger this test case? The patch is gtg as it is since the change is pretty straightforward anyway.
| private static final String CTX_KEY_MAX_SELECTOR_DICTIONARY_SIZE = "maxSelectorDictionarySize"; | ||
| private static final String CTX_KEY_MAX_MERGING_DICTIONARY_SIZE = "maxMergingDictionarySize"; |
There was a problem hiding this comment.
We should mention that this is configurable per query in the Druid's docs as well.
There was a problem hiding this comment.
I've added it to the docs here
any other places I should add it?
There was a problem hiding this comment.
Kinda weird that docs/configuration/index.md says that we can override, even though overriding was a no-op.
Looking up the blame, it is mentioned here #12763, that removing it was intentional. Perhaps we should not add it back then, and fix up the discrepancy above WDYT?
There was a problem hiding this comment.
It was removed from the context as setting it might not be a common case + "complicates calculation"
...however as the tests also rely on similar ways...I went to add it back
I believe its better to have all options follow the same convention... instead of doing something different just for these options...(which have silently caused that this feature become untested)
fyi: @gianm
There was a problem hiding this comment.
Can you also modify the documentation for both the parameters here
druid/docs/configuration/index.md
Line 2164 in 67c7b62
kgyrtkirk
changed the title
|
actually; in all cases around that point in |
|
Thanks for the contribution! |
I think this is a problem as it discards the false return value when the putToKeyBuffer can't store the value because of the limit Not forwarding the return value at that point may lead to the normal continuation here regardless something was not added to the dictionary like here
I think this is a problem as it discards the false return value when the putToKeyBuffer can't store the value because of the limit Not forwarding the return value at that point may lead to the normal continuation here regardless something was not added to the dictionary like here
I think this is a problem as it discards the false return value when the putToKeyBuffer can't store the value because of the limit Not forwarding the return value at that point may lead to the normal continuation here regardless something was not added to the dictionary like here
3 participants
I think this is a problem as it discards the
falsereturn value when theputToKeyBuffercan't store the value because of the limitNot forwarding the return value at that point may lead to the normal continuation here regardless something was not added to the dictionary like here
This PR has: