Skip to content

[SPARK-10648] Proposed bug fix when oracle returns -127 as a scale to a numeric type #8780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

[SPARK-10648] Proposed bug fix when oracle returns -127 as a scale to a numeric type #8780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b0c3be3
Proposed bug fix when oracle returns -127 as a scale to a numeric type
travishegner Sep 16, 2015
01b89bd
Merge remote-tracking branch 'upstream/master'
travishegner Sep 21, 2015
9b1d407
only testing for precision > 0, rather than scale also, since it is l…
travishegner Sep 21, 2015
8f2f3fd
Merge remote-tracking branch 'upstream/master'
travishegner Sep 22, 2015
8110d30
removed any validity checking for decimal/numeric types from JDBCRDD.…
travishegner Sep 22, 2015
d11141c
Merge remote-tracking branch 'upstream/master'
travishegner Sep 29, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/types/DecimalType.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,12 @@ object DecimalType extends AbstractDataType {
}

private[sql] def bounded(precision: Int, scale: Int): DecimalType = {
DecimalType(min(precision, MAX_PRECISION), min(scale, MAX_SCALE))
if (precision <= 0)
Copy link
Contributor

@davies davies Sep 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's risky to change this one, I'd only change the JDBCRDD

Copy link
Contributor Author

@travishegner travishegner Oct 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take your word for the risk involved, I am very new to this project.

From a layman's perspective, it seems that doing some basic checks when instantiating the type would make the type more robust. If I understand correctly a precision <= 0 is not allowed, so this patch returns a /default/ decimal. Similarly, a scale > precision is not allowed, so it returns a decimal with the scale truncated to the size of the precision. My thoughts are that this will catch unexpected inputs and still behave in an expected way. Users instantiating these decimals in the ways that are intended will still get the same type back.

DecimalType.SYSTEM_DEFAULT
else if (scale > precision)
DecimalType(min(precision, MAX_PRECISION), min(precision, MAX_SCALE))
else
DecimalType(min(precision, MAX_PRECISION), min(scale, MAX_SCALE))
}

override private[sql] def defaultConcreteType: DataType = SYSTEM_DEFAULT
Expand Down
8 changes: 2 additions & 6 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRDD.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ private[sql] object JDBCRDD extends Logging {
case java.sql.Types.CLOB => StringType
case java.sql.Types.DATALINK => null
case java.sql.Types.DATE => DateType
case java.sql.Types.DECIMAL
if precision != 0 || scale != 0 => DecimalType.bounded(precision, scale)
case java.sql.Types.DECIMAL => DecimalType.SYSTEM_DEFAULT
case java.sql.Types.DECIMAL => DecimalType.bounded(precision, scale)
case java.sql.Types.DISTINCT => null
Copy link
Contributor

@davies davies Sep 30, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about change this to scale >= 0?

Copy link
Contributor Author

@travishegner travishegner Oct 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't work as demonstrated earlier in the thread. A negative scale is legal to reduce precision to the 10s place, 100s place, etc...

case java.sql.Types.DOUBLE => DoubleType
case java.sql.Types.FLOAT => FloatType
Expand All @@ -80,9 +78,7 @@ private[sql] object JDBCRDD extends Logging {
case java.sql.Types.NCHAR => StringType
case java.sql.Types.NCLOB => StringType
case java.sql.Types.NULL => null
case java.sql.Types.NUMERIC
if precision != 0 || scale != 0 => DecimalType.bounded(precision, scale)
case java.sql.Types.NUMERIC => DecimalType.SYSTEM_DEFAULT
case java.sql.Types.NUMERIC => DecimalType.bounded(precision, scale)
case java.sql.Types.NVARCHAR => StringType
case java.sql.Types.OTHER => null
case java.sql.Types.REAL => DoubleType
Expand Down