Avoid leading and trailing zeros in test_timestamp_seconds_rounding_necessary #9970
Conversation
…cessary Signed-off-by: Haoyang Li <[email protected]>
|
LGTM |
|
build |
| @@ -244,7 +244,7 @@ def start(self, rand): | |||
|
|
|||
| class DecimalGen(DataGen): | |||
| """Generate Decimals, with some built in corner cases.""" | |||
| def __init__(self, precision=None, scale=None, nullable=True, special_cases=None, avoid_positive_values=False): | |||
| def __init__(self, precision=None, scale=None, nullable=True, special_cases=None, avoid_positive_values=False, full_precision=False): | |||
There was a problem hiding this comment.
Could we add a comment/doc string about what full_precision means? It is not clear from just the name of it.
There was a problem hiding this comment.
The first element in the df with the failed seed is 1793879511158.1649100, so it bypasses the rounding necessary
Can you help elaborate this more and put this test comments?
To my understanding, this is a problem when we generating data with tailing/heading zeros, it mislead the overflow exceptions.
Is this an issue of test (data) design or our code?
|
|
||
| @pytest.mark.parametrize('data_gen', [DecimalGen(7, 7), DecimalGen(20, 7)], ids=idfn) | ||
|
|
||
| # Make sure every decimal value in test data is rounding necessary by set full_precision=True to |
There was a problem hiding this comment.
I want to keep it as is because it's the error message, so added a quote around it.
| @pytest.mark.parametrize('data_gen', [DecimalGen(7, 7), DecimalGen(20, 7)], ids=idfn) | ||
|
|
||
| # Make sure every decimal value in test data is rounding necessary by set full_precision=True to | ||
| # avoid leading and trailing zeros |
There was a problem hiding this comment.
Seems trailing zeros is correct, I made a typo in PR title.
Signed-off-by: Haoyang Li <[email protected]>
thirtiseven
changed the title
|
build |
Signed-off-by: Haoyang Li <[email protected]>
I added a test comments. It's an issue of test data. |
|
build |
| @@ -244,7 +244,8 @@ def start(self, rand): | |||
|
|
|||
| class DecimalGen(DataGen): | |||
| """Generate Decimals, with some built in corner cases.""" | |||
| def __init__(self, precision=None, scale=None, nullable=True, special_cases=None, avoid_positive_values=False): | |||
| def __init__(self, precision=None, scale=None, nullable=True, special_cases=None, avoid_positive_values=False, full_precision=False): | |||
| """full_precision: If True, generate decimals with full precision without leading and trailing zeros.""" | |||
There was a problem hiding this comment.
BTW, will trim_zeros be a better name?
There was a problem hiding this comment.
I’m ok with this name, but I personally think trim_zeros means removing leading and trailing zeros from a number, instead of not generating it.
Fixes #9923
timestamp_secondswill check for rounding necessary before overflow, the Decimal(20,7) case satisfies both rounding necessary and overflow, so it should complain about rounding necessary. The first element in the df with the failed seed is 1793879511158.1649100, so it bypasses the rounding necessary check and passes the overflow check next.This PR adds a new parameter
full_precisionin integration testDecimalGento avoid it generating data with leading and tailing zeros, and sets it to True in test_timestamp_seconds_rounding_necessary.