You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by 7shurik <gi...@git.apache.org> on 2015/04/16 01:07:35 UTC
[GitHub] phoenix pull request: Phoenix 1759
GitHub user 7shurik opened a pull request:
https://github.com/apache/phoenix/pull/68
Phoenix 1759
This is a fix for https://issues.apache.org/jira/browse/PHOENIX-1759. There's a set of unit tests as well as an integration test.
Main code change is adding a PhoenixArray.toString() implementation and toString on each primitive array type. There're also a couple of fixes for toStringLiteral not to barf on nulls.
Please let me know if you see any problems.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/7shurik/phoenix PHOENIX-1759
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/68.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 #68
----
commit 9b6a4f049f084edb4d3fb03c960eb37b24d120b2
Author: Alex <aw...@salesforce.com>
Date: 2015-04-15T19:19:08Z
JIRA PHOENIX-1759 - Fix implementation of PhoenixArray toString(). Includes both object and primitive arrays.
commit 0fb879e74a3dc55211046c96f30a054a5aeacb67
Author: Alex <aw...@salesforce.com>
Date: 2015-04-15T19:30:32Z
Add intergration test for array toString()
commit ca6243de97f4bc203278bd129a62e08ffea896f4
Author: Alex <aw...@salesforce.com>
Date: 2015-04-15T23:03:07Z
Integration test for PhoenixArray ResultSet.getString(), also sync PDataType to latest trunk version.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
Github user 7shurik commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28529111
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PhoenixArray.java ---
@@ -561,6 +666,23 @@ public int estimateByteSize(int pos) {
}
@Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder(TO_STRING_BEGIN);
+ boolean isFirst = true;
+ final Format defaultFormat = null;
+ for (Byte o : byteArr) {
+ if (isFirst) {
+ isFirst = false;
+ } else {
+ sb.append(TO_STRING_SEPARATOR);
+ }
+ sb.append(this.baseType.toStringLiteral(o, defaultFormat));
--- End diff --
Good suggestion on toStringLiteral().
On the second comment.. I started to use PhoenixArray.toString() but that doesn't work. Arrays in primitive array subclasses contain primitives and there's an exception if
for(Object o : arr) { ... } is used. If arr consists of primitives (e.g. ints), then for(Integer o : ints) works because type boxing/unboxing automatically does the right cast.
I guess another way would be to have a method that converts from primitive to object. Eg. int to Integer which would make toString generic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
Github user 7shurik commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28532568
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PhoenixArray.java ---
@@ -561,6 +666,23 @@ public int estimateByteSize(int pos) {
}
@Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder(TO_STRING_BEGIN);
+ boolean isFirst = true;
+ final Format defaultFormat = null;
+ for (Byte o : byteArr) {
+ if (isFirst) {
+ isFirst = false;
+ } else {
+ sb.append(TO_STRING_SEPARATOR);
+ }
+ sb.append(this.baseType.toStringLiteral(o, defaultFormat));
--- End diff --
Thank you, @JamesRTaylor! That's the silver bullet I was looking for. I hate duplicating the code like it did.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28524298
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java ---
@@ -1098,8 +1098,10 @@ public String toStringLiteral(byte[] b, int offset, int length, Format formatter
public String toStringLiteral(Object o, Format formatter) {
if (formatter != null) {
return formatter.format(o);
- }
- return o.toString();
+ } else if(null == o) {
--- End diff --
Minor nit: space between if and (
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
Github user 7shurik commented on the pull request:
https://github.com/apache/phoenix/pull/68#issuecomment-96093375
I'm fixing the Apache license header in PrimitiveIntPhoenixArrayToStringTest.java.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28526475
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PhoenixArray.java ---
@@ -561,6 +666,23 @@ public int estimateByteSize(int pos) {
}
@Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder(TO_STRING_BEGIN);
+ boolean isFirst = true;
+ final Format defaultFormat = null;
+ for (Byte o : byteArr) {
+ if (isFirst) {
+ isFirst = false;
+ } else {
+ sb.append(TO_STRING_SEPARATOR);
+ }
+ sb.append(this.baseType.toStringLiteral(o, defaultFormat));
--- End diff --
Also it looks like toStringLiteral() is taking Object. So is it possible to just use the toString() method defined in the super class PhoenixArray. This way you won't have to duplicate code for toString() method for every array type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28526230
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PhoenixArray.java ---
@@ -561,6 +666,23 @@ public int estimateByteSize(int pos) {
}
@Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder(TO_STRING_BEGIN);
+ boolean isFirst = true;
+ final Format defaultFormat = null;
+ for (Byte o : byteArr) {
+ if (isFirst) {
+ isFirst = false;
+ } else {
+ sb.append(TO_STRING_SEPARATOR);
+ }
+ sb.append(this.baseType.toStringLiteral(o, defaultFormat));
--- End diff --
Looks like the defaultFormat is always null. How about having a new method toStringLiteral(o) which delegates to toStringLiteral(o, defaultFormat) where defaultFormat is null?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28529952
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PhoenixArray.java ---
@@ -561,6 +666,23 @@ public int estimateByteSize(int pos) {
}
@Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder(TO_STRING_BEGIN);
+ boolean isFirst = true;
+ final Format defaultFormat = null;
+ for (Byte o : byteArr) {
+ if (isFirst) {
+ isFirst = false;
+ } else {
+ sb.append(TO_STRING_SEPARATOR);
+ }
+ sb.append(this.baseType.toStringLiteral(o, defaultFormat));
--- End diff --
Use these methods instead of relying on unboxing:
PhoenixArray a;
for (int i = 0; i < a.getDimensions(); i++) {
Object o = a.getElement(i);
}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
Github user 7shurik commented on the pull request:
https://github.com/apache/phoenix/pull/68#issuecomment-93865916
@samarthjain @JamesRTaylor are you still reviewing? I've uploaded the version with suggested changes, including a single implementation of PhoenixArray.toString().
Serhiy Bilousov on the JIRA said he might review over the weekend as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
Github user 7shurik commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/68#discussion_r28524479
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/schema/types/PDataType.java ---
@@ -1098,8 +1098,10 @@ public String toStringLiteral(byte[] b, int offset, int length, Format formatter
public String toStringLiteral(Object o, Format formatter) {
if (formatter != null) {
return formatter.format(o);
- }
- return o.toString();
+ } else if(null == o) {
--- End diff --
I'll fix it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
Github user 7shurik closed the pull request at:
https://github.com/apache/phoenix/pull/68
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on the pull request:
https://github.com/apache/phoenix/pull/68#issuecomment-93872039
+1, LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 1759
Posted by 7shurik <gi...@git.apache.org>.
GitHub user 7shurik reopened a pull request:
https://github.com/apache/phoenix/pull/68
Phoenix 1759
This is a fix for https://issues.apache.org/jira/browse/PHOENIX-1759. There's a set of unit tests as well as an integration test.
Main code change is adding a PhoenixArray.toString() implementation and toString on each primitive array type. There're also a couple of fixes for toStringLiteral not to barf on nulls.
Please let me know if you see any problems.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/7shurik/phoenix PHOENIX-1759
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/68.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 #68
----
commit 9b6a4f049f084edb4d3fb03c960eb37b24d120b2
Author: Alex <aw...@salesforce.com>
Date: 2015-04-15T19:19:08Z
JIRA PHOENIX-1759 - Fix implementation of PhoenixArray toString(). Includes both object and primitive arrays.
commit 0fb879e74a3dc55211046c96f30a054a5aeacb67
Author: Alex <aw...@salesforce.com>
Date: 2015-04-15T19:30:32Z
Add intergration test for array toString()
commit ca6243de97f4bc203278bd129a62e08ffea896f4
Author: Alex <aw...@salesforce.com>
Date: 2015-04-15T23:03:07Z
Integration test for PhoenixArray ResultSet.getString(), also sync PDataType to latest trunk version.
commit 1be527d737cc1ade2e121202085c42106e2ad29a
Author: Alex <aw...@salesforce.com>
Date: 2015-04-16T16:58:22Z
Fix formatting, add more PVarcharArray tests.
commit fc47b68ee56e99e35cd0b96e836cb633df003993
Author: Alex <aw...@salesforce.com>
Date: 2015-04-16T17:55:02Z
Unify PhoenixArray.toString() to work for all types of arrays, remove other toString() implementations. Also create toStringLiteral(Object) to use when there's no Formatter.
commit b6de356c0fc49c90a4263e6b36dd848c0fc368b9
Author: Alex <aw...@salesforce.com>
Date: 2015-04-16T17:57:23Z
Fix tabs=>spaces in PhoenixArray.toString()
commit 02a17c57fa18b74c8f97a11eb6446826a1e84b56
Author: Alex <aw...@salesforce.com>
Date: 2015-04-17T22:45:15Z
Merge branch 'master' into PHOENIX-1759
commit 9220dbb4854b9448802cc4361ade5c5c8470d401
Author: Alex <aw...@salesforce.com>
Date: 2015-04-24T23:00:23Z
Fix Apache license header (2=>2.0)
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---