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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8898 )

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

IMPALA-6231: Implement decimal_v2 fuzz test

Implement a test that generates random decimal numbers in the pytest
framework, performs a random mathemtaical operation in Impala and
verifies that the result is correct by doing the same operating using
the Python decimal module. We try to generate not only completely random
decimal numbers, but also numbers that have interesting properties, such
as the number being a power of two.

Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
Reviewed-on: http://gerrit.cloudera.org:8080/8898
Reviewed-by: Michael Brown 
Reviewed-by: Zach Amsden 
Tested-by: Impala Public Jenkins
---
A tests/query_test/test_decimal_fuzz.py
1 file changed, 248 insertions(+), 0 deletions(-)

Approvals:
  Michael Brown: Looks good to me, but someone else must approve
  Zach Amsden: Looks good to me, approved
  Impala Public Jenkins: Verified

--
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: merged
Gerrit-Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
Gerrit-Change-Number: 8898
Gerrit-PatchSet: 4
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 

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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8898 )

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


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1693/


--
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: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 09 Jan 2018 23:17:54 +
Gerrit-HasComments: No

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

[Zach Amsden (Code Review)]

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

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


Patch Set 3: Code-Review+2

Looks great!


--
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: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Tue, 09 Jan 2018 23:11:32 +
Gerrit-HasComments: No

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

[Michael Brown (Code Review)]

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 3: Code-Review+1

Zach, since you were taking a look, too, do you want to give +2 when you're 
satisfied?


--
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: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 08 Jan 2018 21:29:20 +
Gerrit-HasComments: No

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

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/8898 )

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

IMPALA-6231: Implement decimal_v2 fuzz test

Implement a test that generates random decimal numbers in the pytest
framework, performs a random mathemtaical operation in Impala and
verifies that the result is correct by doing the same operating using
the Python decimal module. We try to generate not only completely random
decimal numbers, but also numbers that have interesting properties, such
as the number being a power of two.

Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
---
A tests/query_test/test_decimal_fuzz.py
1 file changed, 248 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8898/3
--
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: newpatchset
Gerrit-Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
Gerrit-Change-Number: 8898
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 

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

[Michael Brown (Code Review)]

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 2:

(4 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@46
PS1, Line 46:
> I'm just trying to remove all test dimensions that we normally use (such as
I have seen this done using two methods.

1. Do not include "vector" in the test method's formal parameter list.

2. Use create_single_exec_option_dimension()


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@70
PS1, Line 70:
> Yes, I think we want 38. It's ok that 38 shows up in both extreme precision
Done


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@230
PS1, Line 230:   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)
 : else:
> Are you suggesting to create a mapping m that maps, for example, "+" to ope
Done


http://gerrit.cloudera.org:8080/#/c/8898/1/tests/query_test/test_decimal_fuzz.py@248
PS1, Line 248:   for _ in xrange(self.itera
> Cool tip, but I decided against making each iteration a separate test. Too
Done



--
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: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Mon, 08 Jan 2018 17:22:16 +
Gerrit-HasComments: Yes

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

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8898 )

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

IMPALA-6231: Implement decimal_v2 fuzz test

Implement a test that generates random decimal numbers in the pytest
framework, performs a random mathemtaical operation in Impala and
verifies that the result is correct by doing the same operating using
the Python decimal module. We try to generate not only completely random
decimal numbers, but also numbers that have interesting properties, such
as the number being a power of two.

Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
---
A tests/query_test/test_decimal_fuzz.py
1 file changed, 249 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/8898/2
--
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: newpatchset
Gerrit-Change-Id: I4328125de5c583ec8ead1f78d9a08703b18b2d85
Gerrit-Change-Number: 8898
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 

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

[Taras Bobrovytsky (Code Review)]

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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Sat, 06 Jan 2018 02:14:28 +
Gerrit-HasComments: Yes

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

[Zach Amsden (Code Review)]

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

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


Patch Set 1:

(3 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 proces
Use def setup_method and teardown_method for this?


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
I think we want 38


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 
repeated fraction results and they checked out  - e.g.,

(Decimal('99.998') / Decimal('999')).quantize(Decimal('0.' + '0' * 8 + '1'))



--
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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 22 Dec 2017 00:35:53 +
Gerrit-HasComments: Yes

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

[Michael Brown (Code Review)]

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 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 21 Dec 2017 21:30:12 +
Gerrit-HasComments: Yes