-
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?
add DECIMAL type support as primary key #3087
Conversation
Summary of ChangesHello @gpalanisamyYopp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
...e/cloud/teleport/v2/source/reader/io/jdbc/uniformsplitter/range/BoundarySplitterFactory.java
Outdated
Show resolved
Hide resolved
...e/cloud/teleport/v2/source/reader/io/jdbc/uniformsplitter/range/BoundarySplitterFactory.java
Show resolved
Hide resolved
…littability checks
...google/cloud/teleport/v2/source/reader/io/jdbc/dialectadapter/mysql/MysqlDialectAdapter.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| BigDecimal decimalStepSize = null; | ||
| if (indexType.equals(IndexType.FLOAT) || indexType.equals(IndexType.DECIMAL)) { |
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 FLOAT type checked in yet.
Or should I hold on this reivew till FLOAT is merged in
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.
This PR has some overlap with the float PR. It would likely be simpler to start with the float PR, and then we will rebase this PR on top of that one.
| BigDecimal decimalStepSize = null; | ||
| if (indexType.equals(IndexType.FLOAT) || indexType.equals(IndexType.DECIMAL)) { | ||
| if (numericScale > 0) { | ||
| decimalStepSize = BigDecimal.ONE.scaleByPowerOfTen(-numericScale); |
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.
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 comment
The 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.
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.
@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 setScale and then unscaledValue to get a corresponding BigInteger, and then split with that). I'm going to work on swapping out the decimal step size in favour of just the scale itself.
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 would also say, I personally think it makes more sense for the SourceColumnIndexInfo class to have just the numeric scale, and then this gets used in various ways depending on the column type (since at the end of the day, the idea of "scale" is the same for decimal, double, and float, we just make use of it in different ways). But that can be a later improvement/refactoring.
|
Could we please fix the build? |
|
@VardhanThigle I unfortunately don't have permissions to push to the fork and the developer that owns this is out of office for the rest of the year. I've re-created the PR but up-to-date with main and with build and issues fixed in #3119. Please close out this PR. |
No description provided.