Skip to content
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

Raise error if maximum prefix expansions are reached #604

Open
wants to merge 3 commits into
base: master
from

Conversation

2 participants
@mnunberg
Copy link
Collaborator

commented Feb 14, 2019

No description provided.

mnunberg added some commits Feb 13, 2019

Raise error if maximum prefix expansions are reached (#579)
This modifies a rather confusing behavior in which prefix expansions are
truncated at a fixed limit (unbeknownst to the user). This behavior can
still be overridden at the configuration level.
Raise prefix expansion limit to 800
We've beefed up performance on union iterators, so we can afford this
now.

@mnunberg mnunberg requested a review from MeirShpilraien Feb 14, 2019

@@ -149,7 +154,7 @@ sds RSConfig_GetInfoString(const RSConfig *config);
#define RS_DEFAULT_CONFIG \
{ \
.concurrentMode = 1, .extLoad = NULL, .enableGC = 1, .minTermPrefix = 2, \
.maxPrefixExpansions = 200, .queryTimeoutMS = 500, .timeoutPolicy = TimeoutPolicy_Return, \
.maxPrefixExpansions = 800, .queryTimeoutMS = 500, .timeoutPolicy = TimeoutPolicy_Return, \

This comment has been minimized.

Copy link
@MeirShpilraien

MeirShpilraien Feb 14, 2019

Collaborator

why you did not add prefixExpansionNoError default value here?

@MeirShpilraien
Copy link
Collaborator

left a comment

Correct me if I am wrong but now people might get error while before this change they did not, this might be an issue, maybe by default we should not raise error?

@mnunberg

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 14, 2019

The change in behavior will be somewhat intentional. I guess the idea is to raise the limit high enough so that it won't matter, or to simply set the config value.

@MeirShpilraien

This comment has been minimized.

Copy link
Collaborator

commented Feb 17, 2019

I think we should not change default behavior but lets discuss it on weekly

@gkorland gkorland added this to In progress in 2.0.0 via automation Jul 17, 2019

@K-Jo K-Jo removed this from In progress in 2.0.0 Jul 17, 2019

@K-Jo K-Jo added this to Pending PR/Merge in 1.6 Jul 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.