You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by tejasapatil <gi...@git.apache.org> on 2017/03/07 02:38:10 UTC
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
GitHub user tejasapatil opened a pull request:
https://github.com/apache/spark/pull/17184
[SPARK-19843] [SQL] UTF8String => (int / long) conversion expensive for invalid inputs
## What changes were proposed in this pull request?
Jira : https://issues.apache.org/jira/browse/SPARK-19843
Added cheap checks to `UTF8String` to check if it can be converted to int / long. The logic is motivated from [LazyUtils.isNumberMaybe in Hive](https://github.com/apache/hive/blob/ff67cdda1c538dc65087878eeba3e165cf3230f4/serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyUtils.java#L90).
## How was this patch tested?
- Added new unit tests
- Ran a prod job which had conversion from string -> int and verified the outputs
## Performance
Attached `UTF8StringBenchmark` with the PR. Here is the summary:
```
conversion to int: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
without check over valid integers 500 / 503 33.6 29.8 1.0X
with check over valid integers 1069 / 1072 15.7 63.7 0.5X
without check over in-valid integers 34515 / 36096 0.5 2057.3 0.0X
with check over in-valid integers 718 / 895 23.4 42.8 0.7X
```
Note that this PR is a tradeoff. If the data does not have any invalid inputs, then performance of conversion is hurt by 2x. Based on the numbers in the benchmark, if more than 2.5% of input cannot be converted to `int`, then it will add perf. gains. For my workloads, I could see on average 37% savings (best case 2.08x performance gain).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tejasapatil/spark SPARK-19843_is_numeric_maybe
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17184.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 #17184
----
commit adcb405bc889345c662ddda1742d3ea95191570d
Author: Tejas Patil <te...@fb.com>
Date: 2017-03-07T00:56:32Z
[SPARK-19843] [SQL] UTF8String => (int / long) conversion expensive for invalid inputs
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74159 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74159/testReport)** for PR 17184 at commit [`7ae2f73`](https://github.com/apache/spark/commit/7ae2f73a50b2abfaa94d7302ae2ff47121057e77).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104821700
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -857,6 +857,55 @@ private int getDigit(byte b) {
throw new NumberFormatException(toString());
}
+ // Mimics LazyUtils.isNumberMaybe() with an additional check for number of bytes
+ private boolean isNumericMaybe(int maxLengthWithoutSign) {
+ if (numBytes <= 0) {
+ // check for empty string
+ return false;
+ }
+
+ int maxNumBytes = maxLengthWithoutSign;
+ byte firstByte = getByte(0);
+
+ if (!Character.isDigit(firstByte)) {
+ // if the first character isn't a digit, then it has to be either `+` OR `-`
+ if ((firstByte == '-' || firstByte == '+')) {
--- End diff --
this code is removed as I have changed the approach
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/74161/
Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104821937
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala ---
@@ -309,8 +307,11 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
// ShortConverter
private[this] def castToShort(from: DataType): Any => Any = from match {
case StringType =>
- buildCast[UTF8String](_, s => try s.toShort catch {
- case _: NumberFormatException => null
+ val result = new IntWrapper()
+ buildCast[UTF8String](_, s => if (s.toShort(result)) {
+ result.getValue.asInstanceOf[Short]
--- End diff --
use `toShort`?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74156 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74156/testReport)** for PR 17184 at commit [`a91ca1b`](https://github.com/apache/spark/commit/a91ca1b15a0dcb6d419fb604c4b0e28feab38753).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` public static class LongWrapper `
* ` public static class IntWrapper `
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/74156/
Test FAILed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on the issue:
https://github.com/apache/spark/pull/17184
Generated code (in case anyone reviewing the PR wants to see):
```
/* 001 */ public Object generate(Object[] references) {
/* 002 */ return new GeneratedIterator(references);
/* 003 */ }
/* 004 */
/* 005 */ final class GeneratedIterator extends org.apache.spark.sql.execution.BufferedRowIterator {
/* 006 */ private Object[] references;
/* 007 */ private scala.collection.Iterator[] inputs;
/* 008 */ private scala.collection.Iterator inputadapter_input;
/* 009 */ private UTF8String.IntWrapper project_wrapper;
/* 010 */ private UTF8String.LongWrapper project_wrapper1;
/* 011 */ private UTF8String.IntWrapper project_wrapper2;
/* 012 */ private UTF8String.IntWrapper project_wrapper3;
/* 013 */ private UnsafeRow project_result;
/* 014 */ private org.apache.spark.sql.catalyst.expressions.codegen.BufferHolder project_holder;
/* 015 */ private org.apache.spark.sql.catalyst.expressions.codegen.UnsafeRowWriter project_rowWriter;
/* 016 */
/* 017 */ public GeneratedIterator(Object[] references) {
/* 018 */ this.references = references;
/* 019 */ }
/* 020 */
/* 021 */ public void init(int index, scala.collection.Iterator[] inputs) {
/* 022 */ partitionIndex = index;
/* 023 */ this.inputs = inputs;
/* 024 */ inputadapter_input = inputs[0];
/* 025 */ project_wrapper = new UTF8String.IntWrapper();
/* 026 */ project_wrapper1 = new UTF8String.LongWrapper();
/* 027 */ project_wrapper2 = new UTF8String.IntWrapper();
/* 028 */ project_wrapper3 = new UTF8String.IntWrapper();
/* 029 */ project_result = new UnsafeRow(4);
/* 030 */ this.project_holder = new org.apache.spark.sql.catalyst.expressions.codegen.BufferHolder(project_result, 0);
/* 031 */ this.project_rowWriter = new org.apache.spark.sql.catalyst.expressions.codegen.UnsafeRowWriter(project_holder, 4);
/* 032 */
/* 033 */ }
/* 034 */
/* 035 */ protected void processNext() throws java.io.IOException {
/* 036 */ while (inputadapter_input.hasNext() && !stopEarly()) {
/* 037 */ InternalRow inputadapter_row = (InternalRow) inputadapter_input.next();
/* 038 */ boolean inputadapter_isNull = inputadapter_row.isNullAt(0);
/* 039 */ UTF8String inputadapter_value = inputadapter_isNull ? null : (inputadapter_row.getUTF8String(0));
/* 040 */
/* 041 */ boolean project_isNull = inputadapter_isNull;
/* 042 */ int project_value = -1;
/* 043 */ if (!inputadapter_isNull) {
/* 044 */ if (inputadapter_value.toInt(project_wrapper)) {
/* 045 */ project_value = project_wrapper.value;
/* 046 */ } else {
/* 047 */ project_isNull = true;
/* 048 */ }
/* 049 */
/* 050 */ }
/* 051 */ boolean inputadapter_isNull1 = inputadapter_row.isNullAt(1);
/* 052 */ UTF8String inputadapter_value1 = inputadapter_isNull1 ? null : (inputadapter_row.getUTF8String(1));
/* 053 */ boolean project_isNull2 = inputadapter_isNull1;
/* 054 */ long project_value2 = -1L;
/* 055 */ if (!inputadapter_isNull1) {
/* 056 */ if (inputadapter_value1.toLong(project_wrapper1)) {
/* 057 */ project_value2 = project_wrapper1.value;
/* 058 */ } else {
/* 059 */ project_isNull2 = true;
/* 060 */ }
/* 061 */
/* 062 */ }
/* 063 */ boolean project_isNull4 = inputadapter_isNull;
/* 064 */ byte project_value4 = (byte)-1;
/* 065 */ if (!inputadapter_isNull) {
/* 066 */ if (inputadapter_value.toByte(project_wrapper2)) {
/* 067 */ project_value4 = (byte) project_wrapper2.value;
/* 068 */ } else {
/* 069 */ project_isNull4 = true;
/* 070 */ }
/* 071 */
/* 072 */ }
/* 073 */ boolean project_isNull6 = inputadapter_isNull;
/* 074 */ short project_value6 = (short)-1;
/* 075 */ if (!inputadapter_isNull) {
/* 076 */ if (inputadapter_value.toShort(project_wrapper3)) {
/* 077 */ project_value6 = (short) project_wrapper3.value;
/* 078 */ } else {
/* 079 */ project_isNull6 = true;
/* 080 */ }
/* 081 */
/* 082 */ }
/* 083 */ project_rowWriter.zeroOutNullBytes();
/* 084 */
/* 085 */ if (project_isNull) {
/* 086 */ project_rowWriter.setNullAt(0);
/* 087 */ } else {
/* 088 */ project_rowWriter.write(0, project_value);
/* 089 */ }
/* 090 */
/* 091 */ if (project_isNull2) {
/* 092 */ project_rowWriter.setNullAt(1);
/* 093 */ } else {
/* 094 */ project_rowWriter.write(1, project_value2);
/* 095 */ }
/* 096 */
/* 097 */ if (project_isNull4) {
/* 098 */ project_rowWriter.setNullAt(2);
/* 099 */ } else {
/* 100 */ project_rowWriter.write(2, project_value4);
/* 101 */ }
/* 102 */
/* 103 */ if (project_isNull6) {
/* 104 */ project_rowWriter.setNullAt(3);
/* 105 */ } else {
/* 106 */ project_rowWriter.write(3, project_value6);
/* 107 */ }
/* 108 */ append(project_result);
/* 109 */ if (shouldStop()) return;
/* 110 */ }
/* 111 */ }
/* 112 */ }
```
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104823136
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -850,26 +850,35 @@ public UTF8String translate(Map<Character, Character> dict) {
return fromString(sb.toString());
}
- private int getDigit(byte b) {
- if (b >= '0' && b <= '9') {
- return b - '0';
+ public static class LongWrapper {
+ private long value = 0;
--- End diff --
removed getter and setter. changed to public
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17184
Ideally we need a better way to indicate an invalid input, as exception handling is too expensive. Adding a check before process is one approach, but as you said, it has performance penalty if all records are valid.
How about we use C-style function and return the result via parameter?. e.g.
```
class ToIntResult {
int res = 0;
boolean valid = true;
}
public void toInt(ToIntResult t) {
...
if (something wrong) {
t.valid = false;
}
...
t.res = xxx;
}
```
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104821560
--- Diff: common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java ---
@@ -608,4 +609,55 @@ public void writeToOutputStreamIntArray() throws IOException {
.writeTo(outputStream);
assertEquals("\u5927\u5343\u4e16\u754c", outputStream.toString("UTF-8"));
}
+
+ @Test
+ public void testIsIntMaybe() throws IOException {
+ assertEquals(true, UTF8String.fromString("1").isIntMaybe());
--- End diff --
changed
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Merged build finished. Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74156 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74156/testReport)** for PR 17184 at commit [`a91ca1b`](https://github.com/apache/spark/commit/a91ca1b15a0dcb6d419fb604c4b0e28feab38753).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on the issue:
https://github.com/apache/spark/pull/17184
also, cc @gatorsmile
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Merged build finished. Test FAILed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104845661
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -897,41 +898,52 @@ public long toLong() {
break;
}
- int digit = getDigit(b);
+ int digit;
+ if (b >= '0' && b <= '9') {
+ digit = b - '0';
+ } else {
+ return false;
+ }
+
// We are going to process the new digit and accumulate the result. However, before doing
// this, if the result is already smaller than the stopValue(Long.MIN_VALUE / radix), then
- // result * 10 will definitely be smaller than minValue, and we can stop and throw exception.
+ // result * 10 will definitely be smaller than minValue, and we can stop.
if (result < stopValue) {
- throw new NumberFormatException(toString());
+ return false;
}
result = result * radix - digit;
// Since the previous result is less than or equal to stopValue(Long.MIN_VALUE / radix), we
- // can just use `result > 0` to check overflow. If result overflows, we should stop and throw
- // exception.
+ // can just use `result > 0` to check overflow. If result overflows, we should stop.
if (result > 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
// This is the case when we've encountered a decimal separator. The fractional
// part will not change the number, but we will verify that the fractional part
// is well formed.
while (offset < numBytes) {
- if (getDigit(getByte(offset)) == -1) {
- throw new NumberFormatException(toString());
+ byte currentByte = getByte(offset);
+ if (currentByte < '0' || currentByte > '9') {
+ return false;
}
offset++;
}
if (!negative) {
result = -result;
if (result < 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
- return result;
+ toLongResult.value = result;
+ return true;
+ }
+
+ public static class IntWrapper {
--- End diff --
OK that's fair. In that case let's document this (otherwise somebody might come in and remove this in the future..)
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104851283
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -850,26 +850,27 @@ public UTF8String translate(Map<Character, Character> dict) {
return fromString(sb.toString());
}
- private int getDigit(byte b) {
- if (b >= '0' && b <= '9') {
- return b - '0';
- }
- throw new NumberFormatException(toString());
+ public static class LongWrapper {
--- End diff --
followup PR : https://github.com/apache/spark/pull/17205
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17184
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on the issue:
https://github.com/apache/spark/pull/17184
Updated the perf. numbers with latest version of code
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104821778
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/UTF8StringBenchmark.scala ---
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+import java.math.BigInteger
+import java.security.SecureRandom
+import java.util.Random
+
+import org.apache.spark.unsafe.types.UTF8String
+import org.apache.spark.util.Benchmark
+
+object UTF8StringBenchmark {
--- End diff --
Removed the benchmark code. Had originally attached in case anyone wanted to verify if the benchmarking was sane
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104842090
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -897,41 +898,52 @@ public long toLong() {
break;
}
- int digit = getDigit(b);
+ int digit;
+ if (b >= '0' && b <= '9') {
+ digit = b - '0';
+ } else {
+ return false;
+ }
+
// We are going to process the new digit and accumulate the result. However, before doing
// this, if the result is already smaller than the stopValue(Long.MIN_VALUE / radix), then
- // result * 10 will definitely be smaller than minValue, and we can stop and throw exception.
+ // result * 10 will definitely be smaller than minValue, and we can stop.
if (result < stopValue) {
- throw new NumberFormatException(toString());
+ return false;
}
result = result * radix - digit;
// Since the previous result is less than or equal to stopValue(Long.MIN_VALUE / radix), we
- // can just use `result > 0` to check overflow. If result overflows, we should stop and throw
- // exception.
+ // can just use `result > 0` to check overflow. If result overflows, we should stop.
if (result > 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
// This is the case when we've encountered a decimal separator. The fractional
// part will not change the number, but we will verify that the fractional part
// is well formed.
while (offset < numBytes) {
- if (getDigit(getByte(offset)) == -1) {
- throw new NumberFormatException(toString());
+ byte currentByte = getByte(offset);
+ if (currentByte < '0' || currentByte > '9') {
+ return false;
}
offset++;
}
if (!negative) {
result = -result;
if (result < 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
- return result;
+ toLongResult.value = result;
+ return true;
+ }
+
+ public static class IntWrapper {
--- End diff --
yea the calculation still use int/long respectively, the wrapper is only used for holding the result. There should be not much performance penalty to always use `LongWrapper`.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74161 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74161/testReport)** for PR 17184 at commit [`51e7e3f`](https://github.com/apache/spark/commit/51e7e3ff315fd8a94fcbf538a64536ea40098d8e).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104843817
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -897,41 +898,52 @@ public long toLong() {
break;
}
- int digit = getDigit(b);
+ int digit;
+ if (b >= '0' && b <= '9') {
+ digit = b - '0';
+ } else {
+ return false;
+ }
+
// We are going to process the new digit and accumulate the result. However, before doing
// this, if the result is already smaller than the stopValue(Long.MIN_VALUE / radix), then
- // result * 10 will definitely be smaller than minValue, and we can stop and throw exception.
+ // result * 10 will definitely be smaller than minValue, and we can stop.
if (result < stopValue) {
- throw new NumberFormatException(toString());
+ return false;
}
result = result * radix - digit;
// Since the previous result is less than or equal to stopValue(Long.MIN_VALUE / radix), we
- // can just use `result > 0` to check overflow. If result overflows, we should stop and throw
- // exception.
+ // can just use `result > 0` to check overflow. If result overflows, we should stop.
if (result > 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
// This is the case when we've encountered a decimal separator. The fractional
// part will not change the number, but we will verify that the fractional part
// is well formed.
while (offset < numBytes) {
- if (getDigit(getByte(offset)) == -1) {
- throw new NumberFormatException(toString());
+ byte currentByte = getByte(offset);
+ if (currentByte < '0' || currentByte > '9') {
+ return false;
}
offset++;
}
if (!negative) {
result = -result;
if (result < 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
- return result;
+ toLongResult.value = result;
+ return true;
+ }
+
+ public static class IntWrapper {
--- End diff --
Using IntWrapper for integers gives better perf. If we use LongWrapper for integers, there would be a conversion needed from long -> int. Its not that big of a difference but given that ints are used heavily in workloads, I dont want to leave that behind.
Here is microbenchmark result for current approach VS using LongWrapper everywhere
```
conversion to int: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
IntWrapper 20397 / 20564 26.3 38.0 1.0X
LongWrapper 20855 / 21530 25.7 38.8 1.0X
```
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the issue:
https://github.com/apache/spark/pull/17184
I believe IBM J9 actually improved this specific case (their JIT handles tons of exceptions better). Oh well -- if only JIT is perfect.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104626687
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -857,6 +857,55 @@ private int getDigit(byte b) {
throw new NumberFormatException(toString());
}
+ // Mimics LazyUtils.isNumberMaybe() with an additional check for number of bytes
+ private boolean isNumericMaybe(int maxLengthWithoutSign) {
+ if (numBytes <= 0) {
+ // check for empty string
+ return false;
+ }
+
+ int maxNumBytes = maxLengthWithoutSign;
+ byte firstByte = getByte(0);
+
+ if (!Character.isDigit(firstByte)) {
+ // if the first character isn't a digit, then it has to be either `+` OR `-`
+ if ((firstByte == '-' || firstByte == '+')) {
--- End diff --
Nit: extra parens here
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104821653
--- Diff: common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java ---
@@ -608,4 +609,55 @@ public void writeToOutputStreamIntArray() throws IOException {
.writeTo(outputStream);
assertEquals("\u5927\u5343\u4e16\u754c", outputStream.toString("UTF-8"));
}
+
+ @Test
+ public void testIsIntMaybe() throws IOException {
+ assertEquals(true, UTF8String.fromString("1").isIntMaybe());
+ assertEquals(true, UTF8String.fromString("+1").isIntMaybe());
+ assertEquals(true, UTF8String.fromString("-1").isIntMaybe());
+ assertEquals(true, UTF8String.fromString("0").isIntMaybe());
+ assertEquals(true, UTF8String.fromString(String.valueOf(Integer.MAX_VALUE)).isIntMaybe());
+ assertEquals(true, UTF8String.fromString("+" + String.valueOf(Integer.MAX_VALUE)).isIntMaybe());
+ assertEquals(true, UTF8String.fromString(String.valueOf(Integer.MIN_VALUE)).isIntMaybe());
+
+ Random rand = new Random();
+ for(int i = 0; i < 10; i++) {
--- End diff --
- fixed linter
- I dont want determinism so not setting seed
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104626500
--- Diff: common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java ---
@@ -608,4 +609,55 @@ public void writeToOutputStreamIntArray() throws IOException {
.writeTo(outputStream);
assertEquals("\u5927\u5343\u4e16\u754c", outputStream.toString("UTF-8"));
}
+
+ @Test
+ public void testIsIntMaybe() throws IOException {
+ assertEquals(true, UTF8String.fromString("1").isIntMaybe());
--- End diff --
`assertTrue`?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Merged build finished. Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104841789
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -897,41 +898,52 @@ public long toLong() {
break;
}
- int digit = getDigit(b);
+ int digit;
+ if (b >= '0' && b <= '9') {
+ digit = b - '0';
+ } else {
+ return false;
+ }
+
// We are going to process the new digit and accumulate the result. However, before doing
// this, if the result is already smaller than the stopValue(Long.MIN_VALUE / radix), then
- // result * 10 will definitely be smaller than minValue, and we can stop and throw exception.
+ // result * 10 will definitely be smaller than minValue, and we can stop.
if (result < stopValue) {
- throw new NumberFormatException(toString());
+ return false;
}
result = result * radix - digit;
// Since the previous result is less than or equal to stopValue(Long.MIN_VALUE / radix), we
- // can just use `result > 0` to check overflow. If result overflows, we should stop and throw
- // exception.
+ // can just use `result > 0` to check overflow. If result overflows, we should stop.
if (result > 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
// This is the case when we've encountered a decimal separator. The fractional
// part will not change the number, but we will verify that the fractional part
// is well formed.
while (offset < numBytes) {
- if (getDigit(getByte(offset)) == -1) {
- throw new NumberFormatException(toString());
+ byte currentByte = getByte(offset);
+ if (currentByte < '0' || currentByte > '9') {
+ return false;
}
offset++;
}
if (!negative) {
result = -result;
if (result < 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
- return result;
+ toLongResult.value = result;
+ return true;
+ }
+
+ public static class IntWrapper {
--- End diff --
actually why bother having an IntWrapper? Can't you use LongWrapper?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/74061/
Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74161 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74161/testReport)** for PR 17184 at commit [`51e7e3f`](https://github.com/apache/spark/commit/51e7e3ff315fd8a94fcbf538a64536ea40098d8e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104821734
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -850,26 +850,35 @@ public UTF8String translate(Map<Character, Character> dict) {
return fromString(sb.toString());
}
- private int getDigit(byte b) {
- if (b >= '0' && b <= '9') {
- return b - '0';
+ public static class LongWrapper {
+ private long value = 0;
--- End diff --
it's for internal use only, shall we make it public? We can probably save some function calls
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Merged build finished. Test FAILed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on the issue:
https://github.com/apache/spark/pull/17184
cc @cloud-fan for review
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74061 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74061/testReport)** for PR 17184 at commit [`adcb405`](https://github.com/apache/spark/commit/adcb405bc889345c662ddda1742d3ea95191570d).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74061 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74061/testReport)** for PR 17184 at commit [`adcb405`](https://github.com/apache/spark/commit/adcb405bc889345c662ddda1742d3ea95191570d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17184
thanks, merging to master!
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17184
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/74159/
Test FAILed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104626903
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/UTF8StringBenchmark.scala ---
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+import java.math.BigInteger
+import java.security.SecureRandom
+import java.util.Random
+
+import org.apache.spark.unsafe.types.UTF8String
+import org.apache.spark.util.Benchmark
+
+object UTF8StringBenchmark {
--- End diff --
Not sure what the convention is, but I don't know if we need to preserve this benchmarking code
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104626620
--- Diff: common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java ---
@@ -608,4 +609,55 @@ public void writeToOutputStreamIntArray() throws IOException {
.writeTo(outputStream);
assertEquals("\u5927\u5343\u4e16\u754c", outputStream.toString("UTF-8"));
}
+
+ @Test
+ public void testIsIntMaybe() throws IOException {
+ assertEquals(true, UTF8String.fromString("1").isIntMaybe());
+ assertEquals(true, UTF8String.fromString("+1").isIntMaybe());
+ assertEquals(true, UTF8String.fromString("-1").isIntMaybe());
+ assertEquals(true, UTF8String.fromString("0").isIntMaybe());
+ assertEquals(true, UTF8String.fromString(String.valueOf(Integer.MAX_VALUE)).isIntMaybe());
+ assertEquals(true, UTF8String.fromString("+" + String.valueOf(Integer.MAX_VALUE)).isIntMaybe());
+ assertEquals(true, UTF8String.fromString(String.valueOf(Integer.MIN_VALUE)).isIntMaybe());
+
+ Random rand = new Random();
+ for(int i = 0; i < 10; i++) {
--- End diff --
Nit, which lint might catch: space after for.
Seed the RNG for determinism?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17184
**[Test build #74159 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/74159/testReport)** for PR 17184 at commit [`7ae2f73`](https://github.com/apache/spark/commit/7ae2f73a50b2abfaa94d7302ae2ff47121057e77).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17184: [SPARK-19843] [SQL] UTF8String => (int / long) conversio...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on the issue:
https://github.com/apache/spark/pull/17184
Jenkins test this please
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104841803
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -850,26 +850,27 @@ public UTF8String translate(Map<Character, Character> dict) {
return fromString(sb.toString());
}
- private int getDigit(byte b) {
- if (b >= '0' && b <= '9') {
- return b - '0';
- }
- throw new NumberFormatException(toString());
+ public static class LongWrapper {
--- End diff --
sure
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104841735
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -850,26 +850,27 @@ public UTF8String translate(Map<Character, Character> dict) {
return fromString(sb.toString());
}
- private int getDigit(byte b) {
- if (b >= '0' && b <= '9') {
- return b - '0';
- }
- throw new NumberFormatException(toString());
+ public static class LongWrapper {
--- End diff --
@tejasapatil can you submit a follow up small pr to add classdoc for this? would be great to explain why we have this.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104841761
--- Diff: common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java ---
@@ -897,41 +898,52 @@ public long toLong() {
break;
}
- int digit = getDigit(b);
+ int digit;
+ if (b >= '0' && b <= '9') {
+ digit = b - '0';
+ } else {
+ return false;
+ }
+
// We are going to process the new digit and accumulate the result. However, before doing
// this, if the result is already smaller than the stopValue(Long.MIN_VALUE / radix), then
- // result * 10 will definitely be smaller than minValue, and we can stop and throw exception.
+ // result * 10 will definitely be smaller than minValue, and we can stop.
if (result < stopValue) {
- throw new NumberFormatException(toString());
+ return false;
}
result = result * radix - digit;
// Since the previous result is less than or equal to stopValue(Long.MIN_VALUE / radix), we
- // can just use `result > 0` to check overflow. If result overflows, we should stop and throw
- // exception.
+ // can just use `result > 0` to check overflow. If result overflows, we should stop.
if (result > 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
// This is the case when we've encountered a decimal separator. The fractional
// part will not change the number, but we will verify that the fractional part
// is well formed.
while (offset < numBytes) {
- if (getDigit(getByte(offset)) == -1) {
- throw new NumberFormatException(toString());
+ byte currentByte = getByte(offset);
+ if (currentByte < '0' || currentByte > '9') {
+ return false;
}
offset++;
}
if (!negative) {
result = -result;
if (result < 0) {
- throw new NumberFormatException(toString());
+ return false;
}
}
- return result;
+ toLongResult.value = result;
+ return true;
+ }
+
+ public static class IntWrapper {
--- End diff --
and perhaps move this closer to LongWrapper
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17184: [SPARK-19843] [SQL] UTF8String => (int / long) co...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/17184#discussion_r104823145
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala ---
@@ -309,8 +307,11 @@ case class Cast(child: Expression, dataType: DataType, timeZoneId: Option[String
// ShortConverter
private[this] def castToShort(from: DataType): Any => Any = from match {
case StringType =>
- buildCast[UTF8String](_, s => try s.toShort catch {
- case _: NumberFormatException => null
+ val result = new IntWrapper()
+ buildCast[UTF8String](_, s => if (s.toShort(result)) {
+ result.getValue.asInstanceOf[Short]
--- End diff --
changed
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org