-
Notifications
You must be signed in to change notification settings - Fork 485
[#6656] fix(catalog): support read time/datetime/timestamp columns with precision #6657
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?
[#6656] fix(catalog): support read time/datetime/timestamp columns with precision #6657
Conversation
...n/java/org/apache/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java
Outdated
Show resolved
Hide resolved
@diqiu50 would you please help to review this PR again? |
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.
How do you handle the type of Timestamp without precision? Should a parameter be added to indicate using the default precision
Hi @diqiu50, I understand your point of view. Yes, I also think that controlling whether to use default precision through parameters is a good way. I will revise it again. |
Gently ping. Are you working on this issue? |
Apologies for the delay! I will prioritize resolving this issue by this week. |
hi @diqiu50, could you please take another look and let me know if everything meets the requirements, or if there's anything else to be adjusted? Thx. |
...c-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java
Outdated
Show resolved
Hide resolved
e525a57
to
d3e8ef5
Compare
hi @diqiu50, I've implemented updates to address the previously failed IT tests. Could you kindly review the latest version when convenient? |
Please fix the error in CI |
...n/java/org/apache/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java
Outdated
Show resolved
Hide resolved
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.
Each catalog has its own default value for the time type, and we should handle this default value when converting the field type of the catalog.
664bda6
to
1359a93
Compare
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.
should we also update the behavior of the Spark connector?
BTW, don't forget to update the user doc and open-api doc
* @return the precision of the time/datetime/timestamp type | ||
*/ | ||
public Integer calculateDatetimePrecision(String typeName, int columnSize, int scale) { | ||
return null; |
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.
why always return null?
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 provides a default behavior for catalogs that do not implement this method.
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.
If so, please add a comment on the method to specify when the developer should return null or non-null.
return Types.TimestampType.withoutTimeZone(); | ||
return Optional.ofNullable(typeBean.getDatetimePrecision()) | ||
.map(Types.TimestampType::withoutTimeZone) | ||
.orElseGet(Types.TimestampType::withoutTimeZone); |
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.
always return Types.TimestampType::withoutTimeZone
?
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.
Yes, Doris only supports the timezone-free time type datetime
...c-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java
Outdated
Show resolved
Hide resolved
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.
could you plz add tests for datetime/timestamp column with specific 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.
fine
// MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back | ||
// from UTC to the current time zone for retrieval. (This does not occur for other types | ||
// such as DATETIME.) see more details: | ||
// https://dev.mysql.com/doc/refman/8.0/en/datetime.html | ||
case TIMESTAMP: | ||
return Types.TimestampType.withTimeZone(); | ||
return Optional.ofNullable(typeBean.getDatetimePrecision()) |
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.
when will typeBean.getDatetimePrecision()
return null?
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.
from our implementation, it should not be possible
default: | ||
throw new TrinoException( | ||
GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT, | ||
"Invalid MySQL timestamp precision: " + precision + ". Valid values are 0, 3, 6"); |
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 can add a comment to describe the source of this behavior.
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.
BTW, after this PR merged, we also need to update the Python client (since #7241 has been merged), so could you plz open an new issue to track this?
2c59e0c
to
28febe9
Compare
Signed-off-by: zacsun <[email protected]>
28febe9
to
9d76ca2
Compare
Signed-off-by: zacsun <[email protected]>
| `Double` | `Double` | | ||
| `Decimal` | `Decimal` | | ||
| `Date` | `Date` | | ||
| `Timestamp[p]` | `Datetime[p]` | |
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.
Timestamp(p)
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.
missing the doc
| Timestamp | `Types.TimestampType.withoutTimeZone()` | `timestamp` | Timestamp type, indicates a timestamp without timezone | |
Why are the changes needed?
Fix: #6656
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT already added.