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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Comment-Date: Sat, 06 Jan 2018 02:14:28 +0000 Gerrit-HasComments: Yes
