Add work around for string split with empty input. #11292
Merged
Conversation
This file contains 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
Signed-off-by: Robert (Bobby) Evans <bobby@apache.org>
jlowe
previously approved these changes
Aug 2, 2024
Comment on lines
1058
to
1059
| else: | ||
| return StringGen("", nullable=True) |
There was a problem hiding this comment.
Nit: defensive programming
Suggested change
| else: | |
| return StringGen("", nullable=True) | |
| elif (empty_type == EmptyStringType.MIXED): | |
| return StringGen("", nullable=True) | |
| else: | |
| raise AssertionError("unexpected empty type " + str(empty_type)) |
or alternatively make it a map lookup, e.g.:
empty_string_gens_map = {
EmptyStringType.ALL_NULL : lambda: NullGen(StringType()),
EmptyStringType.ALL_EMPTY : lambda: StringGen("", nullable=False)
EmptyStringType.MIXED : lambda: StringGen("", nullable=True)
}
def mk_empty_str_gen(empty_type):
return empty_string_gens_map[empty_type]()
Comment on lines
443
to
446
| @pytest.mark.parametrize('empty_type', [ | ||
| EmptyStringType.ALL_NULL, | ||
| EmptyStringType.ALL_EMPTY, | ||
| EmptyStringType.MIXED]) |
There was a problem hiding this comment.
Suggested change
| @pytest.mark.parametrize('empty_type', [ | |
| EmptyStringType.ALL_NULL, | |
| EmptyStringType.ALL_EMPTY, | |
| EmptyStringType.MIXED]) | |
| @pytest.mark.parametrize('empty_type', list(EmptyStringType.__members__)) |
Comment on lines
33
to
36
| @pytest.mark.parametrize('empty_type', [ | ||
| EmptyStringType.ALL_NULL, | ||
| EmptyStringType.ALL_EMPTY, | ||
| EmptyStringType.MIXED]) |
There was a problem hiding this comment.
Suggested change
| @pytest.mark.parametrize('empty_type', [ | |
| EmptyStringType.ALL_NULL, | |
| EmptyStringType.ALL_EMPTY, | |
| EmptyStringType.MIXED]) | |
| @pytest.mark.parametrize('empty_type', list(EmptyStringType.__members__)) |
|
build |
|
@jlowe please take another look |
jlowe
approved these changes
Aug 2, 2024
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.
This fixes #11287
I traced down all of the string split calls mentioned in the original CUDF issue rapidsai/cudf#16453.
I added some tests for
GpuStringToMap, but I could not trigger the issue there. I also added in a test formarraybecause I saw some similar odd behavior forGpuStringSplitwhen the number of splits = 1, which didn't callstringSplit*at all, but I also could not trigger any issues there.I didn't touch HiveTextFile because an empty file felt invalid, but I can go back and try to test it if we want to.