[BugFix] Create temporary partitions bugfix + fix comparisons on Type.Date and Type.Datetime #60014
+60
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
23 tasks
alvin-celerdata
previously approved these changes
Jun 18, 2025
|
@mergify rebase |
❌ Pull request can't be updated with latest base branch changesMergify needs the author permission to update the base branch of the pull request. |
|
@ctbrennan, the failed UT has been fixed. Could you rebase to the main branch to pass the CI check? |
da4c82f to
d6813c8
Compare
yes, done |
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 6 / 6 (100.00%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
alvin-celerdata
approved these changes
Jun 20, 2025
Seaven
approved these changes
Jun 20, 2025
|
@Mergifyio backport branch-3.3 |
|
@Mergifyio backport branch-3.5 |
|
@Mergifyio backport branch-3.4 |
✅ Backports have been created
|
✅ Backports have been created
|
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Jun 20, 2025
….Date and Type.Datetime (#60014) Why I'm doing: When adding temporary partitions using an ALTER TABLE statement with START and END values that include hour/minute/second, StarRocks would incorrectly truncate the partition key values and only preserve the date part after parsing, even if the partition column is of type DATETIME. This leads to errors when adding hourly temporary partitions, including failed validation due to "The lower values must smaller than upper values." Logging at each step showed that: The table and partition column’s type remained DATETIME throughout the parser, visitor, and analyzer. However, inside MultiRangePartitionDesc.buildDateTypePartition(), even though the type was reported as DATETIME, the code used the DATE formatter unless the object passed an identity (==) comparison to Type.DATETIME. Note on what I learned during unit test writing I found by forcibly creating a non-singleton ScalarType in tests—that there must exist at least one code path in the system where a ScalarType (such as DATETIME) is instantiated without going through the canonical singleton factory method (Type.DATETIME, Type.fromPrimitiveType, etc.). This probably happens during some sort of image deserialization, by json/protobuf libraries maybe. The added unit test demonstrates this risk. While this PR addresses the specific instance I found, similar issues could exist in other type comparison sites throughout the codebase, hard to say. What I'm doing: Replaced == with .isDatetime() when comparing partition column types to Type.DATETIME in date partition construction logic. For this, the relevant one is in com.starrocks.sql.ast.MultiRangePartitionDesc#buildDateTypePartition. I also went and did the same thing where there was obviously the same bug for Date and Datetime types. Another possible solution would have been to override Type.equals() but I'm not confident that I'd do it correctly for all cases and it would have a bigger blast radius. Temporary partitions now correctly parse and preserve full datetime values, supporting hourly partitioning aligned with the schema. Fixes #issue (cherry picked from commit 5f25005)
22 tasks
mergify bot
pushed a commit
that referenced
this pull request
Jun 20, 2025
….Date and Type.Datetime (#60014) Why I'm doing: When adding temporary partitions using an ALTER TABLE statement with START and END values that include hour/minute/second, StarRocks would incorrectly truncate the partition key values and only preserve the date part after parsing, even if the partition column is of type DATETIME. This leads to errors when adding hourly temporary partitions, including failed validation due to "The lower values must smaller than upper values." Logging at each step showed that: The table and partition column’s type remained DATETIME throughout the parser, visitor, and analyzer. However, inside MultiRangePartitionDesc.buildDateTypePartition(), even though the type was reported as DATETIME, the code used the DATE formatter unless the object passed an identity (==) comparison to Type.DATETIME. Note on what I learned during unit test writing I found by forcibly creating a non-singleton ScalarType in tests—that there must exist at least one code path in the system where a ScalarType (such as DATETIME) is instantiated without going through the canonical singleton factory method (Type.DATETIME, Type.fromPrimitiveType, etc.). This probably happens during some sort of image deserialization, by json/protobuf libraries maybe. The added unit test demonstrates this risk. While this PR addresses the specific instance I found, similar issues could exist in other type comparison sites throughout the codebase, hard to say. What I'm doing: Replaced == with .isDatetime() when comparing partition column types to Type.DATETIME in date partition construction logic. For this, the relevant one is in com.starrocks.sql.ast.MultiRangePartitionDesc#buildDateTypePartition. I also went and did the same thing where there was obviously the same bug for Date and Datetime types. Another possible solution would have been to override Type.equals() but I'm not confident that I'd do it correctly for all cases and it would have a bigger blast radius. Temporary partitions now correctly parse and preserve full datetime values, supporting hourly partitioning aligned with the schema. Fixes #issue (cherry picked from commit 5f25005)
mergify bot
pushed a commit
that referenced
this pull request
Jun 20, 2025
….Date and Type.Datetime (#60014) Why I'm doing: When adding temporary partitions using an ALTER TABLE statement with START and END values that include hour/minute/second, StarRocks would incorrectly truncate the partition key values and only preserve the date part after parsing, even if the partition column is of type DATETIME. This leads to errors when adding hourly temporary partitions, including failed validation due to "The lower values must smaller than upper values." Logging at each step showed that: The table and partition column’s type remained DATETIME throughout the parser, visitor, and analyzer. However, inside MultiRangePartitionDesc.buildDateTypePartition(), even though the type was reported as DATETIME, the code used the DATE formatter unless the object passed an identity (==) comparison to Type.DATETIME. Note on what I learned during unit test writing I found by forcibly creating a non-singleton ScalarType in tests—that there must exist at least one code path in the system where a ScalarType (such as DATETIME) is instantiated without going through the canonical singleton factory method (Type.DATETIME, Type.fromPrimitiveType, etc.). This probably happens during some sort of image deserialization, by json/protobuf libraries maybe. The added unit test demonstrates this risk. While this PR addresses the specific instance I found, similar issues could exist in other type comparison sites throughout the codebase, hard to say. What I'm doing: Replaced == with .isDatetime() when comparing partition c 6D40 olumn types to Type.DATETIME in date partition construction logic. For this, the relevant one is in com.starrocks.sql.ast.MultiRangePartitionDesc#buildDateTypePartition. I also went and did the same thing where there was obviously the same bug for Date and Datetime types. Another possible solution would have been to override Type.equals() but I'm not confident that I'd do it correctly for all cases and it would have a bigger blast radius. Temporary partitions now correctly parse and preserve full datetime values, supporting hourly partitioning aligned with the schema. Fixes #issue (cherry picked from commit 5f25005)
ctbrennan
added a commit
to pinterest/starrocks
that referenced
this pull request
Jun 20, 2025
….Date and Type.Datetime (StarRocks#60014) Why I'm doing: When adding temporary partitions using an ALTER TABLE statement with START and END values that include hour/minute/second, StarRocks would incorrectly truncate the partition key values and only preserve the date part after parsing, even if the partition column is of type DATETIME. This leads to errors when adding hourly temporary partitions, including failed validation due to "The lower values must smaller than upper values." Logging at each step showed that: The table and partition column’s type remained DATETIME throughout the parser, visitor, and analyzer. However, inside MultiRangePartitionDesc.buildDateTypePartition(), even though the type was reported as DATETIME, the code used the DATE formatter unless the object passed an identity (==) comparison to Type.DATETIME. Note on what I learned during unit test writing I found by forcibly creating a non-singleton ScalarType in tests—that there must exist at least one code path in the system where a ScalarType (such as DATETIME) is instantiated without going through the canonical singleton factory method (Type.DATETIME, Type.fromPrimitiveType, etc.). This probably happens during some sort of image deserialization, by json/protobuf libraries maybe. The added unit test demonstrates this risk. While this PR addresses the specific instance I found, similar issues could exist in other type comparison sites throughout the codebase, hard to say. What I'm doing: Replaced == with .isDatetime() when comparing partition column types to Type.DATETIME in date partition construction logic. For this, the relevant one is in com.starrocks.sql.ast.MultiRangePartitionDesc#buildDateTypePartition. I also went and did the same thing where there was obviously the same bug for Date and Datetime types. Another possible solution would have been to override Type.equals() but I'm not confident that I'd do it correctly for all cases and it would have a bigger blast radius. Temporary partitions now correctly parse and preserve full datetime values, supporting hourly partitioning aligned with the schema. Fixes #issue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why I'm doing:
When adding temporary partitions using an ALTER TABLE statement with START and END values that include hour/minute/second, StarRocks would incorrectly truncate the partition key values and only preserve the date part after parsing, even if the partition column is of type DATETIME. This leads to errors when adding hourly temporary partitions, including failed validation due to "The lower values must smaller than upper values."
Note on what I learned during unit test writing
I found by forcibly creating a non-singleton ScalarType in tests—that there must exist at least one code path in the system where a ScalarType (such as DATETIME) is instantiated without going through the canonical singleton factory method (Type.DATETIME, Type.fromPrimitiveType, etc.). This probably happens during some sort of image deserialization, by json/protobuf libraries maybe.
The added unit test demonstrates this risk. While this PR addresses the specific instance I found, similar issues could exist in other type comparison sites throughout the codebase, hard to say.
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: