You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Yash Sharma <ya...@gmail.com> on 2014/08/05 23:54:15 UTC
Review Request 24345: Drill-1141 - IsNumeric function review request
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/
-----------------------------------------------------------
Review request for drill, Aditya Kishore and Mehant Baid.
Repository: drill-git
Description
-------
Implemented Drill Function IsNumeric for DRILL-1141
Diffs
-----
exec/java-exec/src/main/codegen/config.fmpp ff6135d
exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
Diff: https://reviews.apache.org/r/24345/diff/
Testing
-------
Yes.
Test Case:
$mvn test -Dtest=TestNewMathFunctions#testIsNumeric
Sqlline Test:
0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
+-------------+-------------+------------+-------------+
| employee_id | isnumeric_1 | first_name | isnumeric_2 |
+-------------+-------------+------------+-------------+
| 1 | 1 | Sheri | 0 |
| 2 | 1 | Derrick | 0 |
| 4 | 1 | Michael | 0 |
| 5 | 1 | Maya | 0 |
| 6 | 1 | Roberta | 0 |
| 7 | 1 | Rebecca | 0 |
| 8 | 1 | Kim | 0 |
| 9 | 1 | Brenda | 0 |
| 10 | 1 | Darren | 0 |
| 11 | 1 | Jonathan | 0 |
+-------------+-------------+------------+-------------+
10 rows selected (0.451 seconds)
Thanks,
Yash Sharma
Re: Review Request 24345: Drill-1141 - IsNumeric function review request
Posted by Yash Sharma <ya...@gmail.com>.
> On Aug. 7, 2014, 12:23 a.m., Mehant Baid wrote:
> > exec/java-exec/src/main/codegen/data/NumericTypes.tdd, line 40
> > <https://reviews.apache.org/r/24345/diff/1/?file=652801#file652801line40>
> >
> > You don't have to generate functions for nullable data types since you are using NULL_IF_NULL annotation in your function template.
Removed NULL_IF_NULL. Now returning 0 for null values.
- Yash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/#review49840
-----------------------------------------------------------
On Aug. 5, 2014, 9:54 p.m., Yash Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24345/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 9:54 p.m.)
>
>
> Review request for drill, Aditya Kishore and Mehant Baid.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Implemented Drill Function IsNumeric for DRILL-1141
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/config.fmpp ff6135d
> exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
> exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
> exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24345/diff/
>
>
> Testing
> -------
>
> Yes.
>
>
> Test Case:
>
> $mvn test -Dtest=TestNewMathFunctions#testIsNumeric
>
> Sqlline Test:
>
> 0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
>
> +-------------+-------------+------------+-------------+
> | employee_id | isnumeric_1 | first_name | isnumeric_2 |
> +-------------+-------------+------------+-------------+
> | 1 | 1 | Sheri | 0 |
> | 2 | 1 | Derrick | 0 |
> | 4 | 1 | Michael | 0 |
> | 5 | 1 | Maya | 0 |
> | 6 | 1 | Roberta | 0 |
> | 7 | 1 | Rebecca | 0 |
> | 8 | 1 | Kim | 0 |
> | 9 | 1 | Brenda | 0 |
> | 10 | 1 | Darren | 0 |
> | 11 | 1 | Jonathan | 0 |
> +-------------+-------------+------------+-------------+
> 10 rows selected (0.451 seconds)
>
>
> Thanks,
>
> Yash Sharma
>
>
Re: Review Request 24345: Drill-1141 - IsNumeric function review request
Posted by Mehant Baid <ba...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/#review49840
-----------------------------------------------------------
exec/java-exec/src/main/codegen/data/NumericTypes.tdd
<https://reviews.apache.org/r/24345/#comment87230>
You don't have to generate functions for nullable data types since you are using NULL_IF_NULL annotation in your function template.
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java
<https://reviews.apache.org/r/24345/#comment87231>
If its already a numeric data type (int, bigint, float4 ...) you don't have to convert it to string, apply the regex etc. You can simply return 1.
- Mehant Baid
On Aug. 5, 2014, 9:54 p.m., Yash Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24345/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 9:54 p.m.)
>
>
> Review request for drill, Aditya Kishore and Mehant Baid.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Implemented Drill Function IsNumeric for DRILL-1141
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/config.fmpp ff6135d
> exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
> exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
> exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24345/diff/
>
>
> Testing
> -------
>
> Yes.
>
>
> Test Case:
>
> $mvn test -Dtest=TestNewMathFunctions#testIsNumeric
>
> Sqlline Test:
>
> 0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
>
> +-------------+-------------+------------+-------------+
> | employee_id | isnumeric_1 | first_name | isnumeric_2 |
> +-------------+-------------+------------+-------------+
> | 1 | 1 | Sheri | 0 |
> | 2 | 1 | Derrick | 0 |
> | 4 | 1 | Michael | 0 |
> | 5 | 1 | Maya | 0 |
> | 6 | 1 | Roberta | 0 |
> | 7 | 1 | Rebecca | 0 |
> | 8 | 1 | Kim | 0 |
> | 9 | 1 | Brenda | 0 |
> | 10 | 1 | Darren | 0 |
> | 11 | 1 | Jonathan | 0 |
> +-------------+-------------+------------+-------------+
> 10 rows selected (0.451 seconds)
>
>
> Thanks,
>
> Yash Sharma
>
>
Re: Review Request 24345: Drill-1141 - IsNumeric function review
request
Posted by Yash Sharma <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/
-----------------------------------------------------------
(Updated Nov. 12, 2014, 6:07 p.m.)
Review request for drill, Aditya Kishore and Mehant Baid.
Changes
-------
Rebased Patch
Repository: drill-git
Description
-------
Implemented Drill Function IsNumeric for DRILL-1141
Diffs (updated)
-----
exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java f715747
exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
Diff: https://reviews.apache.org/r/24345/diff/
Testing (updated)
-------
Yes.
Test Case:
$mvn test -Dtest=TestNewMathFunctions#testIsNumeric
Sqlline Test:
0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
+-------------+-------------+------------+-------------+
| employee_id | isnumeric_1 | first_name | isnumeric_2 |
+-------------+-------------+------------+-------------+
| 1 | 1 | Sheri | 0 |
| 2 | 1 | Derrick | 0 |
| 4 | 1 | Michael | 0 |
| 5 | 1 | Maya | 0 |
| 6 | 1 | Roberta | 0 |
| 7 | 1 | Rebecca | 0 |
| 8 | 1 | Kim | 0 |
| 9 | 1 | Brenda | 0 |
| 10 | 1 | Darren | 0 |
| 11 | 1 | Jonathan | 0 |
+-------------+-------------+------------+-------------+
10 rows selected (0.451 seconds)
Thanks,
Yash Sharma
Re: Review Request 24345: Drill-1141 - IsNumeric function review
request
Posted by Mehant Baid <ba...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/#review51749
-----------------------------------------------------------
Ship it!
Ship It!
- Mehant Baid
On Aug. 7, 2014, 9:05 a.m., Yash Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24345/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 9:05 a.m.)
>
>
> Review request for drill, Aditya Kishore and Mehant Baid.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Implemented Drill Function IsNumeric for DRILL-1141
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/config.fmpp ff6135d
> exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
> exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
> exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24345/diff/
>
>
> Testing
> -------
>
> Yes.
>
>
> Test Case:
>
> $mvn test -Dtest=TestNewMathFunctions#testIsNumeric
>
> Sqlline Test:
>
> 0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
>
> +-------------+-------------+------------+-------------+
> | employee_id | isnumeric_1 | first_name | isnumeric_2 |
> +-------------+-------------+------------+-------------+
> | 1 | 1 | Sheri | 0 |
> | 2 | 1 | Derrick | 0 |
> | 4 | 1 | Michael | 0 |
> | 5 | 1 | Maya | 0 |
> | 6 | 1 | Roberta | 0 |
> | 7 | 1 | Rebecca | 0 |
> | 8 | 1 | Kim | 0 |
> | 9 | 1 | Brenda | 0 |
> | 10 | 1 | Darren | 0 |
> | 11 | 1 | Jonathan | 0 |
> +-------------+-------------+------------+-------------+
> 10 rows selected (0.451 seconds)
>
>
> Thanks,
>
> Yash Sharma
>
>
Re: Review Request 24345: Drill-1141 - IsNumeric function review request
Posted by Yash Sharma <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 9:05 a.m.)
Review request for drill, Aditya Kishore and Mehant Baid.
Repository: drill-git
Description
-------
Implemented Drill Function IsNumeric for DRILL-1141
Diffs (updated)
-----
exec/java-exec/src/main/codegen/config.fmpp ff6135d
exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
Diff: https://reviews.apache.org/r/24345/diff/
Testing
-------
Yes.
Test Case:
$mvn test -Dtest=TestNewMathFunctions#testIsNumeric
Sqlline Test:
0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
+-------------+-------------+------------+-------------+
| employee_id | isnumeric_1 | first_name | isnumeric_2 |
+-------------+-------------+------------+-------------+
| 1 | 1 | Sheri | 0 |
| 2 | 1 | Derrick | 0 |
| 4 | 1 | Michael | 0 |
| 5 | 1 | Maya | 0 |
| 6 | 1 | Roberta | 0 |
| 7 | 1 | Rebecca | 0 |
| 8 | 1 | Kim | 0 |
| 9 | 1 | Brenda | 0 |
| 10 | 1 | Darren | 0 |
| 11 | 1 | Jonathan | 0 |
+-------------+-------------+------------+-------------+
10 rows selected (0.451 seconds)
Thanks,
Yash Sharma
Re: Review Request 24345: Drill-1141 - IsNumeric function review request
Posted by Yash Sharma <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 8:33 a.m.)
Review request for drill, Aditya Kishore and Mehant Baid.
Changes
-------
Implemented review comments !
Repository: drill-git
Description
-------
Implemented Drill Function IsNumeric for DRILL-1141
Diffs (updated)
-----
exec/java-exec/src/main/codegen/config.fmpp ff6135d
exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
Diff: https://reviews.apache.org/r/24345/diff/
Testing
-------
Yes.
Test Case:
$mvn test -Dtest=TestNewMathFunctions#testIsNumeric
Sqlline Test:
0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
+-------------+-------------+------------+-------------+
| employee_id | isnumeric_1 | first_name | isnumeric_2 |
+-------------+-------------+------------+-------------+
| 1 | 1 | Sheri | 0 |
| 2 | 1 | Derrick | 0 |
| 4 | 1 | Michael | 0 |
| 5 | 1 | Maya | 0 |
| 6 | 1 | Roberta | 0 |
| 7 | 1 | Rebecca | 0 |
| 8 | 1 | Kim | 0 |
| 9 | 1 | Brenda | 0 |
| 10 | 1 | Darren | 0 |
| 11 | 1 | Jonathan | 0 |
+-------------+-------------+------------+-------------+
10 rows selected (0.451 seconds)
Thanks,
Yash Sharma
Re: Review Request 24345: Drill-1141 - IsNumeric function review request
Posted by Yash Sharma <ya...@gmail.com>.
> On Aug. 7, 2014, 1:01 a.m., Jinfeng Ni wrote:
> > exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java, line 85
> > <https://reviews.apache.org/r/24345/diff/1/?file=652802#file652802line85>
> >
> > I'm not sure if JVM will optimize or not. Here, for each input value, the code will compile a regex pattern, and do the match. Seems it's a big overhead.
> >
> > Maybe you can consider move the build of regex pattern into setup() method, since the pattern string in this case is a constant, so that the pattern is only built once for the whole record batch, in stead of for each input value.
> >
> > You may take a look at StringFunctions.like() or similar() operator.
> >
Pattern moved to workspace level. Also reusing Matcher instead of creating new matcher every time.
- Yash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/#review49822
-----------------------------------------------------------
On Aug. 5, 2014, 9:54 p.m., Yash Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24345/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 9:54 p.m.)
>
>
> Review request for drill, Aditya Kishore and Mehant Baid.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Implemented Drill Function IsNumeric for DRILL-1141
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/config.fmpp ff6135d
> exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
> exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
> exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24345/diff/
>
>
> Testing
> -------
>
> Yes.
>
>
> Test Case:
>
> $mvn test -Dtest=TestNewMathFunctions#testIsNumeric
>
> Sqlline Test:
>
> 0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
>
> +-------------+-------------+------------+-------------+
> | employee_id | isnumeric_1 | first_name | isnumeric_2 |
> +-------------+-------------+------------+-------------+
> | 1 | 1 | Sheri | 0 |
> | 2 | 1 | Derrick | 0 |
> | 4 | 1 | Michael | 0 |
> | 5 | 1 | Maya | 0 |
> | 6 | 1 | Roberta | 0 |
> | 7 | 1 | Rebecca | 0 |
> | 8 | 1 | Kim | 0 |
> | 9 | 1 | Brenda | 0 |
> | 10 | 1 | Darren | 0 |
> | 11 | 1 | Jonathan | 0 |
> +-------------+-------------+------------+-------------+
> 10 rows selected (0.451 seconds)
>
>
> Thanks,
>
> Yash Sharma
>
>
Re: Review Request 24345: Drill-1141 - IsNumeric function review request
Posted by Jinfeng Ni <jn...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24345/#review49822
-----------------------------------------------------------
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java
<https://reviews.apache.org/r/24345/#comment87184>
This is a question. What is the value of is numeric() when input is null? Does it make sense to use NULL_IF_NULL? I thought is numeric() will always return a true/false value.
exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java
<https://reviews.apache.org/r/24345/#comment87182>
I'm not sure if JVM will optimize or not. Here, for each input value, the code will compile a regex pattern, and do the match. Seems it's a big overhead.
Maybe you can consider move the build of regex pattern into setup() method, since the pattern string in this case is a constant, so that the pattern is only built once for the whole record batch, in stead of for each input value.
You may take a look at StringFunctions.like() or similar() operator.
- Jinfeng Ni
On Aug. 5, 2014, 2:54 p.m., Yash Sharma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24345/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 2:54 p.m.)
>
>
> Review request for drill, Aditya Kishore and Mehant Baid.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Implemented Drill Function IsNumeric for DRILL-1141
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/codegen/config.fmpp ff6135d
> exec/java-exec/src/main/codegen/data/NumericTypes.tdd f37a3dd
> exec/java-exec/src/main/codegen/templates/NumericFunctionsTemplates.java PRE-CREATION
> exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java 62a224e
> exec/java-exec/src/test/resources/functions/testIsNumericFunction.json PRE-CREATION
>
> Diff: https://reviews.apache.org/r/24345/diff/
>
>
> Testing
> -------
>
> Yes.
>
>
> Test Case:
>
> $mvn test -Dtest=TestNewMathFunctions#testIsNumeric
>
> Sqlline Test:
>
> 0: jdbc:drill:zk=local> SELECT employee_id, isnumeric(employee_id) isnumeric_1, first_name, isnumeric(first_name) isnumeric_2 FROM cp.`employee.json` limit 10;
>
> +-------------+-------------+------------+-------------+
> | employee_id | isnumeric_1 | first_name | isnumeric_2 |
> +-------------+-------------+------------+-------------+
> | 1 | 1 | Sheri | 0 |
> | 2 | 1 | Derrick | 0 |
> | 4 | 1 | Michael | 0 |
> | 5 | 1 | Maya | 0 |
> | 6 | 1 | Roberta | 0 |
> | 7 | 1 | Rebecca | 0 |
> | 8 | 1 | Kim | 0 |
> | 9 | 1 | Brenda | 0 |
> | 10 | 1 | Darren | 0 |
> | 11 | 1 | Jonathan | 0 |
> +-------------+-------------+------------+-------------+
> 10 rows selected (0.451 seconds)
>
>
> Thanks,
>
> Yash Sharma
>
>