impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6231: Implement decimal v2 fuzz test
Date Thu, 21 Dec 2017 21:30:12 GMT
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8898 )

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


Patch Set 1:

(7 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
Will this create a side-effect for the entire lifetime of the python process? We might want
a yield fixture that sets this and then resets it when the tests are complete.


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?


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 string values of (value,
precision, scale)"?


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@70
PS1, Line 70: 38
Would it be better to have 37 here instead, or do you want 38 to be part of the random_precision?


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. Maybe to clear it
up, call this "corner_case_decimal_value" or "interesting_decimal_value" ? I suppose the same
could be said of binary_value.


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 do this dispatching
yourself.

https://docs.python.org/release/2.6.9/library/operator.html#module-operator


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.
 
  @pytest.mark.parametrize('i', xrange(iterations))
  def test_fuzz(self, i, vector):
    self.execute_one()

This makes each iteration a separate test, which may or may not be acceptable to you.

You would have to adjust how iterations is determined, though, so it may or may not be worth
the trouble. Up to you.



-- 
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Dec 2017 21:30:12 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message