You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/10/15 23:15:19 UTC
[GitHub] [phoenix] yanxinyi opened a new pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
yanxinyi opened a new pull request #924:
URL: https://github.com/apache/phoenix/pull/924
…contain column name instead of actual value
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r506773406
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/exception/DataExceedsCapacityException.java
##########
@@ -31,11 +31,26 @@ public DataExceedsCapacityException(String message) {
SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY).setMessage(message).build().buildException());
}
+ @Deprecated
public DataExceedsCapacityException(PDataType type, Integer precision, Integer scale, String columnName, ImmutableBytesWritable value) {
super(new SQLExceptionInfo.Builder(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY)
.setMessage((columnName == null ? "" : columnName + " ") + getTypeDisplayString(type, precision, scale, value))
.build().buildException());
}
+
+ public DataExceedsCapacityException(String columnName, String mesage) {
+ super(new SQLExceptionInfo.Builder(
+ SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY)
+ .setColumnName(columnName)
+ .setMessage(mesage).build()
+ .buildException());
+ }
+
+ public DataExceedsCapacityException(String columnName, int columnLength) {
+ this(columnName, "maxLength=" + columnLength);
+ }
+
+ @Deprecated
Review comment:
You could also deprecate the method `private static String getTypeDisplayString(PDataType type, Integer precision, Integer scale, ImmutableBytesWritable value)` below to avoid accidental usage in future, and add a new helper method that doesn't take `value` as the argument.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] yanxinyi closed pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
yanxinyi closed pull request #924:
URL: https://github.com/apache/phoenix/pull/924
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r506022114
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/schema/MutationTest.java
##########
@@ -92,18 +92,24 @@ public void testSizeConstraint() throws Exception {
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(),e.getErrorCode());
+ assertFalse(e.getMessage().contains("abcd"));
Review comment:
Nit: Could assign "abcd" to a string variable and use that here and on line 91.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #924:
URL: https://github.com/apache/phoenix/pull/924#issuecomment-710716742
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 9s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 33m 15s | 4.x passed |
| +1 :green_heart: | compile | 1m 7s | 4.x passed |
| +1 :green_heart: | checkstyle | 1m 32s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 54s | 4.x passed |
| +0 :ok: | spotbugs | 3m 33s | phoenix-core in 4.x has 956 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 29m 31s | the patch passed |
| +1 :green_heart: | compile | 1m 8s | the patch passed |
| +1 :green_heart: | javac | 1m 8s | the patch passed |
| -1 :x: | checkstyle | 1m 30s | phoenix-core: The patch generated 18 new + 3423 unchanged - 23 fixed = 3441 total (was 3446) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 53s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 51s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 183m 51s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 30s | The patch does not generate ASF License warnings. |
| | | 265m 29s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.AbsFunctionEnd2EndIT |
| | phoenix.end2end.IndexToolForNonTxGlobalIndexIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/924 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux ebfb891a5b3e 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / 2359f54 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/2/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/2/testReport/ |
| Max. process+thread count | 5864 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/2/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r507907536
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/UpsertSelectIT.java
##########
@@ -1677,6 +1677,8 @@ private void testUpsertSelectWithMultiByteChars(boolean autoCommit) throws Excep
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(), e.getErrorCode());
+ assertFalse(e.getMessage().contains("澴粖蟤य褻酃岤豦팑薰鄩脼ժ끦碉碉碉碉碉碉碉"));
+ assertTrue(e.getMessage().contains(String.valueOf(20)));
Review comment:
nit: You could check for `VARCHAR(20)` instead of just `20`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r506773763
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/expression/DecimalAddExpression.java
##########
@@ -74,7 +74,7 @@ public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
result = NumberUtil.setDecimalWidthAndScale(result, maxLength, scale);
}
if (result == null) {
- throw new DataExceedsCapacityException(PDecimal.INSTANCE, maxLength, scale);
+ throw new DataExceedsCapacityException("", maxLength);
Review comment:
The type information could be useful here, and below too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r507911806
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/schema/MutationTest.java
##########
@@ -92,18 +92,24 @@ public void testSizeConstraint() throws Exception {
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(),e.getErrorCode());
+ assertFalse(e.getMessage().contains("abcd"));
+ assertTrue(e.getMessage().contains(String.valueOf(maxLength1)));
}
try {
conn.createStatement().execute("UPSERT INTO t1(k1,v2) VALUES('b','" + value + "')");
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(),e.getErrorCode());
+ assertFalse(e.getMessage().contains(value));
+ assertTrue(e.getMessage().contains(String.valueOf(maxLength2)));
Review comment:
nit: Same here.
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/schema/MutationTest.java
##########
@@ -92,18 +92,24 @@ public void testSizeConstraint() throws Exception {
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(),e.getErrorCode());
+ assertFalse(e.getMessage().contains("abcd"));
+ assertTrue(e.getMessage().contains(String.valueOf(maxLength1)));
}
try {
conn.createStatement().execute("UPSERT INTO t1(k1,v2) VALUES('b','" + value + "')");
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(),e.getErrorCode());
+ assertFalse(e.getMessage().contains(value));
+ assertTrue(e.getMessage().contains(String.valueOf(maxLength2)));
Review comment:
nit: Same here, and below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r506773711
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/exception/DataExceedsCapacityException.java
##########
@@ -31,11 +31,26 @@ public DataExceedsCapacityException(String message) {
SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY).setMessage(message).build().buildException());
}
+ @Deprecated
public DataExceedsCapacityException(PDataType type, Integer precision, Integer scale, String columnName, ImmutableBytesWritable value) {
super(new SQLExceptionInfo.Builder(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY)
.setMessage((columnName == null ? "" : columnName + " ") + getTypeDisplayString(type, precision, scale, value))
.build().buildException());
}
+
+ public DataExceedsCapacityException(String columnName, String mesage) {
+ super(new SQLExceptionInfo.Builder(
+ SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY)
+ .setColumnName(columnName)
+ .setMessage(mesage).build()
+ .buildException());
+ }
+
+ public DataExceedsCapacityException(String columnName, int columnLength) {
+ this(columnName, "maxLength=" + columnLength);
+ }
+
+ @Deprecated
public DataExceedsCapacityException(PDataType type, Integer precision, Integer scale) {
this(type, precision, scale, null, null);
Review comment:
You would also want to change this to call the new constructor: `this(type, precision, scale, null)` since this will be deprecated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r507911659
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/schema/MutationTest.java
##########
@@ -92,18 +92,24 @@ public void testSizeConstraint() throws Exception {
fail();
} catch (SQLException e) {
assertEquals(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY.getErrorCode(),e.getErrorCode());
+ assertFalse(e.getMessage().contains("abcd"));
+ assertTrue(e.getMessage().contains(String.valueOf(maxLength1)));
Review comment:
nit: Same here. Could include the datatype too in the exception message and not just the length.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r506773075
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/exception/DataExceedsCapacityException.java
##########
@@ -31,11 +31,26 @@ public DataExceedsCapacityException(String message) {
SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY).setMessage(message).build().buildException());
}
+ @Deprecated
public DataExceedsCapacityException(PDataType type, Integer precision, Integer scale, String columnName, ImmutableBytesWritable value) {
super(new SQLExceptionInfo.Builder(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY)
.setMessage((columnName == null ? "" : columnName + " ") + getTypeDisplayString(type, precision, scale, value))
.build().buildException());
}
+
Review comment:
You could instead add the new constructor as:
```
public DataExceedsCapacityException(PDataType type, Integer precision, Integer scale, String columnName) {
super(new SQLExceptionInfo.Builder(SQLExceptionCode.DATA_EXCEEDS_MAX_CAPACITY)
.setMessage((columnName == null ? "" : columnName + " ") + getTypeDisplayString(type, precision, scale, null))
.build().buildException());
}
```
And, for example, throw the exception as:
```
throw new DataExceedsCapacityException(column.getDataType(), column.getMaxLength(),
column.getScale(), column.getName().getString());
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] ntshmah commented on a change in pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
ntshmah commented on a change in pull request #924:
URL: https://github.com/apache/phoenix/pull/924#discussion_r506773763
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/expression/DecimalAddExpression.java
##########
@@ -74,7 +74,7 @@ public boolean evaluate(Tuple tuple, ImmutableBytesWritable ptr) {
result = NumberUtil.setDecimalWidthAndScale(result, maxLength, scale);
}
if (result == null) {
- throw new DataExceedsCapacityException(PDecimal.INSTANCE, maxLength, scale);
+ throw new DataExceedsCapacityException("", maxLength);
Review comment:
The type information is useful here, and below too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #924: PHOENIX-6189 DATA_EXCEEDS_MAX_CAPACITY exception error string should …
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #924:
URL: https://github.com/apache/phoenix/pull/924#issuecomment-709713405
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 6m 16s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 33m 43s | 4.x passed |
| +1 :green_heart: | compile | 1m 7s | 4.x passed |
| +1 :green_heart: | checkstyle | 0m 51s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 54s | 4.x passed |
| +0 :ok: | spotbugs | 3m 48s | phoenix-core in 4.x has 956 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 30m 15s | the patch passed |
| +1 :green_heart: | compile | 1m 13s | the patch passed |
| +1 :green_heart: | javac | 1m 13s | the patch passed |
| -1 :x: | checkstyle | 0m 53s | phoenix-core: The patch generated 18 new + 1110 unchanged - 14 fixed = 1128 total (was 1124) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 52s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 53s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 182m 22s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 31s | The patch does not generate ASF License warnings. |
| | | 269m 20s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.rpc.PhoenixClientRpcIT |
| | phoenix.end2end.index.ImmutableIndexExtendedIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/924 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux c68e06ad496a 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / 264310b |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/1/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/1/testReport/ |
| Max. process+thread count | 5658 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-924/1/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org