You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by reudismam <gi...@git.apache.org> on 2018/01/24 12:58:29 UTC
[GitHub] drill pull request #1099: Use valueOf method instead of constructor since va...
GitHub user reudismam opened a pull request:
https://github.com/apache/drill/pull/1099
Use valueOf method instead of constructor since valueOf has a higher …
…performance by caching frequently requested values.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/reudismam/drill master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1099.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1099
----
commit e86d3a209cf5740a13383472b8880faaa8dc6137
Author: reudismam <re...@...>
Date: 2018-01-24T12:36:04Z
Use valueOf method instead of constructor since valueOf has a higher performance by caching frequently requested values.
----
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam you can leave `valueOf` for Double and Float for consistency.
There are two other comments @vrozov has left. Could you please address hem as well? They are not directly related to your changes but will make Drill code better. Also please squash commits into one.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam I guess you can apply back your change for `SSLConfigClient.java` class.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
Conflicts resolved. Do you want to revert the edits to Double and Float? I think it is ok for consistency.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
Only pass Travis CI by removing the edits to SSLConfigClient.java
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam the idea of the fix seems reasonable but there are many other places in code where constructor is used instead of `valueOf`. I suggest you revisit all classes rather then doing partial fix.
---
[GitHub] drill pull request #1099: DRILL-6106: Use valueOf method instead of construc...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1099
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam Travis fails in other PRs as well. See #1105.
---
[GitHub] drill pull request #1099: DRILL-6106: Use valueOf method instead of construc...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on a diff in the pull request:
https://github.com/apache/drill/pull/1099#discussion_r164076111
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigClient.java ---
@@ -94,7 +94,7 @@ private String getStringProperty(String name, String defaultValue) {
private int getIntProperty(String name, int defaultValue) {
int value = defaultValue;
if ( (properties != null) && (properties.containsKey(name))) {
- value = new Integer(properties.getProperty(name)).intValue();
+ value = Integer.valueOf(properties.getProperty(name)).intValue();
--- End diff --
According to Java documentation, there are benefits in using valueOf for Double and Float.
https://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#valueOf(double)
https://docs.oracle.com/javase/7/docs/api/java/lang/Float.html#valueOf(float)
---
[GitHub] drill pull request #1099: DRILL-6106: Use valueOf method instead of construc...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1099#discussion_r164010530
--- Diff: contrib/storage-hbase/src/test/java/org/apache/drill/hbase/TestOrderedBytesConvertFunctions.java ---
@@ -54,7 +54,7 @@ public void testOrderedBytesDouble() throws Throwable {
@Test
public void testOrderedBytesDoubleConvertFrom() throws Throwable {
- verifyPhysicalPlan("convert_from(binary_string('\\x31\\x80\\x00\\x00\\x00\\x00\\x00\\x00\\x01'), 'DOUBLE_OB')", new Double(4.9e-324));
+ verifyPhysicalPlan("convert_from(binary_string('\\x31\\x80\\x00\\x00\\x00\\x00\\x00\\x00\\x01'), 'DOUBLE_OB')", Double.valueOf(4.9e-324));
--- End diff --
For `Double` and `Float` there is no benefit using `valueOf()`, OK if used for consistency.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
I reverted the edit because the code does not pass in Travis CI.
---
[GitHub] drill pull request #1099: DRILL-6106: Use valueOf method instead of construc...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1099#discussion_r164012734
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigClient.java ---
@@ -94,7 +94,7 @@ private String getStringProperty(String name, String defaultValue) {
private int getIntProperty(String name, int defaultValue) {
int value = defaultValue;
if ( (properties != null) && (properties.containsKey(name))) {
- value = new Integer(properties.getProperty(name)).intValue();
+ value = Integer.valueOf(properties.getProperty(name)).intValue();
--- End diff --
```
if (properties != null) {
String property = properties.getProperty(name);
if (property != null && property.length() > 0) {
value = Integer.decode(property);
}
```
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
Well, you can always use force push to override your previous changes or even replace your remote branch with new local.
---
[GitHub] drill pull request #1099: DRILL-6106: Use valueOf method instead of construc...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1099#discussion_r164010865
--- Diff: contrib/storage-hive/core/src/main/codegen/templates/ObjectInspectors.java ---
@@ -169,9 +169,9 @@ public Boolean getPrimitiveJavaObject(Object o) {
if (o == null) {
return null;
}
- return new Boolean(((NullableBitHolder)o).value == 0 ? false : true);
+ return Boolean.valueOf(((NullableBitHolder)o).value == 0 ? false : true);
--- End diff --
either `Boolean.valueOf(((NullableBitHolder)o).value != 0);` or `((NullableBitHolder)o).value == 0 ? Boolean.FALSE : Boolean.TRUE;`
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
I have edited the PR to include the Jira prefix.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
Maybe it has not worked as expected. It squashed the commit (first commit), but as the commit mix commits from other persons, they come together. Maybe it will be the case of creating a patch file for the desired commit and apply this patch to a new pull request.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
LGTM, @vrozov?
---
[GitHub] drill pull request #1099: DRILL-6106: Use valueOf method instead of construc...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1099#discussion_r164165401
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ssl/SSLConfigClient.java ---
@@ -94,7 +94,7 @@ private String getStringProperty(String name, String defaultValue) {
private int getIntProperty(String name, int defaultValue) {
int value = defaultValue;
if ( (properties != null) && (properties.containsKey(name))) {
- value = new Integer(properties.getProperty(name)).intValue();
+ value = Integer.valueOf(properties.getProperty(name)).intValue();
--- End diff --
Do not always trust Java doc :). Here is the actual implementation:
```
public static Double valueOf(double d) {
return new Double(d);
}
```
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1099
LGTM, please squash.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam could you please address code review comments so changes can be pushed into master? Also please resolve merge conflict.
---
[GitHub] drill issue #1099: Use valueOf method instead of constructor since valueOf h...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1099
Hi @reudismam ,
Could you file a JIRA for this PR? Also, you'll need to modify the PR's subject/title to be prefixed with that JIRA number. You can see other PRs for an example.
This allows us to track and approve PRs in the right process.
@arina-ielchiieva , could you take a look at this?
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1099
What was the reason to revert the change in SSLConfigClient.java?
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
I have squashed the commits, but I’m getting an error in Travis CI similar to the previous one when I reverted some changes.
Column a-offsets of type UInt4Vector: Offset (0) must be 0 but was 1
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam classes in protocol package are generated using .proto files, you can not change them.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
Reverted changes to classes in the protocol package.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1099
@reudismam For drill, merging/squashing on github is not an option (due to the way how Apache Drill git is setup). Please rebase and squash.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by reudismam <gi...@git.apache.org>.
Github user reudismam commented on the issue:
https://github.com/apache/drill/pull/1099
Since I have to resolve some conflicts after applying the edit, squashing could be tricky because the commits are non-consecutive. I allowed the merging using the squashing option in the forked project setting. Thus, when someone merges the pull request, he or she can decide to squash the commits.
---
[GitHub] drill issue #1099: DRILL-6106: Use valueOf method instead of constructor sin...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1099
Travis CI failed due to a known issue `TestSortImpl.testLargeBatch:502->runJumboBatchTest:475->runLargeSortTest:454 Value of 1:0 expected:<0> but was:<1>` unrelated to the SSLConfigClient.java changes.
---