Skip to content
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

[core][bug] Fix the incorrect partition key format generated from the DATE type #853

Closed
wants to merge 2 commits into from

Conversation

yuanoOo
Copy link

@yuanoOo yuanoOo commented Apr 9, 2023

fix issue: #773

(Please specify the module before the PR name: [core] ... or [flink] ...)

Purpose

(What is the purpose of the change, or the associated issue)

Tests

(List UT and IT cases to verify this change)

API and Format

(Does this change affect API or storage format)

Documentation

(Does this change introduce a new feature)

@yuanoOo yuanoOo changed the title [core][bug] Fix the incorrect partition key format generated from the… [core][bug] Fix the incorrect partition key format generated from the DATE type Apr 9, 2023
@yuanoOo yuanoOo force-pushed the fix-date-type-partition-key branch 3 times, most recently from d953639 to fe9fc1d Compare April 9, 2023 08:36
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

String partitionValue;

if (Objects.nonNull(field)) {
if (fieldTypes.get(i).is(DataTypeRoot.DATE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

also check Time Type

@yuanoOo yuanoOo force-pushed the fix-date-type-partition-key branch from fe9fc1d to b4d5b12 Compare April 10, 2023 13:14
@yuanoOo yuanoOo force-pushed the fix-date-type-partition-key branch from b4d5b12 to 4872ad1 Compare April 10, 2023 13:20
@JingsongLi
Copy link
Contributor

I've thought about this fix, maybe we can make it more thorough. Like org.apache.paimon.utils.TypeUtils.castFromString, we can have a castToString. Then, the string will be SQL standard string.

But this will bring more serious issues, incompatibility, and we may need an option or an old class to fall back to the old path when we cannot find it. This is complicated.

Is this problem serious now? Can we keep it this way for now?

@yuanoOo
Copy link
Author

yuanoOo commented Apr 11, 2023

I've thought about this fix, maybe we can make it more thorough. Like org.apache.paimon.utils.TypeUtils.castFromString, we can have a castToString. Then, the string will be SQL standard string.

But this will bring more serious issues, incompatibility, and we may need an option or an old class to fall back to the old path when we cannot find it. This is complicated.

Is this problem serious now? Can we keep it this way for now?

I think it's a very user experience affecting issue.
In a stream writing paimon scenario, it is very common to cast a field of timestamp type to DATE type in order to write to the corresponding partition.
However, in paimon, when the user uses a partition of type DATE, the partition path becomes a strange unreadable string of numbers, which may be unacceptable. If you want to get the correct partition string, but need to convert timestamp type to date string, which is counterintuitive and sql is tedious.

I think the current addition of support for DATE type partitioning is sufficient.

For the compatibility issue, it will be easy to solve if we can get the version information of paimon. For example, we can determine how to format DATE type partitions based on the version number. I think there are probably few users who use it for the strange performance of the current version of the DATE type partition.

@SteNicholas
Copy link
Member

@yuanoOo, the partition key format is user-defined that the format like yyyyMMdd could convert from the date or timestamp functions via users. It doesn't need to use fixed format in Paimon. Meanwhile, solving the compatibility issue via version doesn't mark sense to me because it's cumbersome to maintain the compatibility solution.

@yuanoOo yuanoOo closed this May 19, 2023
@dailai
Copy link

dailai commented Mar 18, 2024

I think this issue need to fix. For example, we develop a paimon connector of seatunnel which use java api of paimon also has the same problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants