[Impala-ASF-CR] IMPALA-6231: Implement decimal v2 fuzz test

["Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>]

Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8898 )

Change subject: IMPALA-6231: Implement decimal_v2 fuzz test
......................................................................


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py
File tests/query_test/test_decimal_fuzz.py:

http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@33
PS1, Line 33:   # Set the Python decimal context to a large precision initially, so that the
            :   # mathematical operations are performed at a high precision.
            :   decimal.getcontext().prec = 80
            :   decimal.getcontext().rounding = decimal.ROUND_HALF_UP
> Use def setup_method and teardown_method for this?
I used local decimal context to solve the issue.


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@46
PS1, Line 46: create_exec_option_dimension_from_dict({'_': ['_']})
> Sorry, what does this do?
I'm just trying to remove all test dimensions that we normally use (such as file format). I'm just adding a useless dimension here, because the test doesn't run with zero dimensions. Is there a better way to do this?


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@65
PS1, Line 65: Returns a decimal with a random precision, scale and value.
> What about something a little more precise, like "Return a 3-tuple with str
Done


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@70
PS1, Line 70: 38
> I think we want 38
Yes, I think we want 38. It's ok that 38 shows up in both extreme precision and random precision tests. We want random to be completely random and all values should be possible.


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@134
PS1, Line 134: decimal_value
> It was a little confusing at first to know what the intention was here. May
Done


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@204
PS1, Line 204:       truncated_expected = expected.quantize(
> Nice.  I didn't know you could do this.  I tried a few of the more bizarre 
Yeah, it's pretty cool. I learned about this while developing this test.


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@230
PS1, Line 230:       if op == '+':
             :         expected_result = decimal.Decimal(value1) + decimal.Decimal(value2)
             :       elif op == '-':
             :         expected_result = decimal.Decimal(value1) - decimal.Decimal(value2)
             :       elif op == '*':
             :         expected_result = decimal.Decimal(value1) * decimal.Decimal(value2)
             :       elif op == '/':
             :         expected_result = decimal.Decimal(value1) / decimal.Decimal(value2)
             :       elif op == '%':
             :         expected_result = decimal.Decimal(value1) % decimal.Decimal(value2)
> Up to you, but you could consider the operator module so you don't have to 
Are you suggesting to create a mapping m that maps, for example, "+" to operator.add()? Then calculate result like this m[op](value1, value2)?

I don't think doing this is much better than the current approach.


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@248
PS1, Line 248: def test_fuzz(self, vector):
> You could parallelize the test by making the iteration a test parameter.
Cool tip, but I decided against making each iteration a separate test. Too much stuff gets printed to screen.



-- 
To view, visit http://gerrit.cloudera.org:8080/8898
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
Gerrit-Change-Number: 8898
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Jan 2018 02:14:28 +0000
Gerrit-HasComments: Yes