Escape feature name before using it in a regex in get_split_value_his… - #12389
Open
Nityahapani wants to merge 3 commits into
Open
Escape feature name before using it in a regex in get_split_value_his…#12389Nityahapani wants to merge 3 commits into
Nityahapani wants to merge 3 commits into
Conversation
…togram
get_split_value_histogram() parses the text tree dump looking for
lines matching a feature's split condition, using a regex built via:
regexp = re.compile(r"\[{0}<([\d.Ee+-]+)\]".format(feature))
The feature name is inserted directly into the pattern with no
escaping. Feature names are ordinary user-controlled strings and can
contain regex metacharacters -- parentheses, brackets, +, *, etc --
which is not an exotic edge case: real-world column names like
"price ($)", "x[1]", "col+extra", or "col(1)" are all plausible.
Concretely, for a feature named "price ($)" with an actual split on
it in the dump text "[price ($)<5.5]", the current (unescaped) regex
fails to match at all -- the parentheses are interpreted as a regex
group rather than literal characters -- silently returning an empty
histogram (or triggering the "doesn't support categorical split"
error path) instead of the real split-value data.
Fix: wrap the feature name in re.escape() before formatting it into
the pattern.
Verified standalone (import xgboost requires the compiled C++ core,
which isn't buildable in this sandbox) against a range of realistic
feature names ("price ($)", "a.b", "x[1]", "col+extra", "a*b",
"col(1)", "50%_off", plus an ordinary "plain_feature"):
- before the fix: 5 of 7 names with metacharacters silently fail
to match a dump line that should match
- after the fix: every name, including the plain one (no
regression), matches correctly
mypy and ruff both pass on the modified file.
Member
|
Could you please help add a small test for this? |
Per review feedback from trivialfis: add a test for the regex escape fix. test_split_value_histogram_regex_special_feature_name trains two otherwise-identical models on the same digits dataset -- one with default feature names, one where feature 28 (the same feature already exercised by the existing test_split_value_histograms test) is renamed to "f(28)[+.]", containing parentheses, brackets, a plus sign, and a dot -- all regex metacharacters. Since training params have no randomness sources (no subsample/ colsample, no seed needed for determinism), both models train identically except for the renamed feature, so their split-value histograms for that feature should come out byte-for-byte identical. The test asserts exactly that, plus a direct sanity check that the histogram is non-empty -- which is the actual symptom of the bug: the unescaped regex silently returned an empty histogram for this feature name instead of matching the real split data.
Contributor
Author
|
Added test_split_value_histogram_regex_special_feature_name right after the existing test_split_value_histograms test. It trains two otherwise-identical models on the digits dataset — one with default feature names, one where the same feature (f28, already covered by the existing test) is renamed to "f(28)[+.]", hitting parens, brackets, +, and . all at once. Since there's no randomness in the training params, both models train identically except for the name, so I assert their histograms for that feature come out byte-for-byte the same — plus a direct check that the histogram is non-empty, which is the actual symptom: before the fix, the unescaped regex silently returned empty for this name. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Escape feature name before using it in a regex in get_split_value_histogram
Summary
get_split_value_histogram()parses the text tree dump looking for linesmatching a feature's split condition, using a regex built via:
The feature name is inserted directly into the pattern with no
escaping. Feature names are ordinary user-controlled strings and can
contain regex metacharacters — parentheses, brackets,
+,*, etc — whichis not an exotic edge case: real-world column names like
"price ($)","x[1]","col+extra", or"col(1)"are all plausible.Concrete impact
For a feature named
"price ($)"with an actual split on it in the dumptext
[price ($)<5.5], the current (unescaped) regex fails to match atall — the parentheses are interpreted as a regex group rather than
literal characters — silently returning an empty histogram (or triggering
the
"doesn't support categorical split"error path) instead of the realsplit-value data.
Change
One line — wrap the feature name in
re.escape()before formatting itinto the pattern.
Verification
match a dump line that should match
matches correctly
mypyandruffboth pass on the modified file.