-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Support DECIMAL and remove DOUBLE in TPCH Connector #11166
Conversation
@@ -5,9 +5,17 @@ | |||
<parent> |
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.
Changes made to this file will be reverted prior to merging. Please ignore these changes
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.
Please squash all the fixup commits. Do all the tests are passing?
@@ -5,9 +5,17 @@ | |||
<parent> | |||
<groupId>io.airlift</groupId> | |||
<artifactId>airbase</artifactId> | |||
<version>80</version> |
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.
it looks like you depends on other PR which is in progress, it should be a separate commit (cherry-picked)
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.
Please see:
#11166 (comment)
It is hard for me to locally separate out this commit, and my local testing needs these changes. Hence I raised this as part of the PR, will revert before merging. Please ignore any changes to this file
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.
Please see:
#11166 (comment)
It is hard for me to locally separate out this commit, and my local testing needs these changes. Hence I raised this as part of the PR, will revert before merging. Please ignore any changes to this file
<name>Local AirLift Repo</name> | ||
<url>/Users/atrisharma/tpch/target</url> | ||
</repository> | ||
</repositories> |
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.
?
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.
<artifactId>tpch</artifactId> | ||
<version>0.9</version> | ||
</dependency> | ||
|
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 not move
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.
case DOUBLE: | ||
return column.getDouble(row); | ||
case DECIMAL: | ||
return column.getIdentifier(row); |
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.
did you mean long
?
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.
getIdentifier
returns the long value of the column, should it not work for this case?
case DOUBLE: | ||
return DOUBLE; | ||
case DECIMAL: | ||
return 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.
you should return DECIMAL(12,2), use com.facebook.presto.spi.type.DecimalType#createDecimalType(int, int)
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.
Ack
@@ -29,6 +29,7 @@ | |||
{ | |||
public static final int DEFAULT_SCALE = 0; | |||
public static final int DEFAULT_PRECISION = MAX_PRECISION; | |||
public static final DecimalType DECIMAL = createDecimalType(12, 2); |
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.
please undo
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.
Types usually have a static created type (e.g. DOUBLE etc), hence I created one for DECIMAL. I will use createDecimal
everywhere instead
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 was following the usual pattern of types having a static instance which can be returned. I will use createDecimal
I think some classes in You might want to try running: |
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions! |
Fixes #3557
Airlift PR: trinodb/tpch#23