-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add DECIMAL type support as primary key #3087
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
c5f8afd
8e61113
7a53eed
0135ab7
67b250d
a46ee4e
8d9d652
6503b26
dc932d8
6a31107
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -376,6 +376,7 @@ private ImmutableMap<String, SourceColumnType> getTableCols( | |
| .put("TINYTEXT", IndexType.STRING) | ||
| .put("DATETIME", IndexType.TIME_STAMP) | ||
| .put("TIMESTAMP", IndexType.TIME_STAMP) | ||
| .put("DECIMAL", IndexType.DECIMAL) | ||
| .put("YEAR", IndexType.NUMERIC) | ||
| .build(); | ||
|
|
||
|
|
@@ -452,6 +453,16 @@ private ImmutableList<SourceColumnIndexInfo> getTableIndexes( | |
| stringMaxLength = null; | ||
| } | ||
|
|
||
| BigDecimal decimalStepSize = null; | ||
| if (indexType.equals(IndexType.FLOAT) || indexType.equals(IndexType.DECIMAL)) { | ||
| if (numericScale > 0) { | ||
| decimalStepSize = BigDecimal.ONE.scaleByPowerOfTen(-numericScale); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need to couple this with stepSize or could we handle the scale as is? My worry here is any rounding error in this exponentiation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scale is explicitly required to calculate the StepSize, as the StepSize is used to calculate the smallest possible difference between two values (later on in the Splitter) and then check for inequality of two values by comparing the difference to the smallest possible difference between two values. I don't see a way to decouple these two concepts, but I also don't expect to have rounding issues given we are using a BigDecimal and a Java built-in method to perform the exponentiation. As I understand the BigDecimal type, it is explicitly designed to handle such calculations without loss of precision.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @VardhanThigle I think I see what you mean, and I expect working with the scale directly would actually simplify things a bit (I'm thinking it should allow us to use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would also say, I personally think it makes more sense for the |
||
| } else { | ||
| decimalStepSize = BigDecimal.ONE; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| indexesBuilder.add( | ||
| SourceColumnIndexInfo.builder() | ||
| .setColumnName(colName) | ||
|
|
@@ -463,6 +474,7 @@ private ImmutableList<SourceColumnIndexInfo> getTableIndexes( | |
| .setIndexType(indexType) | ||
| .setCollationReference(collationReference) | ||
| .setStringMaxLength(stringMaxLength) | ||
| .setDecimalStepSize(decimalStepSize) | ||
| .build()); | ||
| } | ||
| } catch (java.sql.SQLException e) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if we have
FLOATtype checked in yet.Or should I hold on this reivew till
FLOATis merged inThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has some overlap with the
floatPR. It would likely be simpler to start with thefloatPR, and then we will rebase this PR on top of that one.