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

[BugFix] check time type in DDL (#23473) #23474

Merged
merged 1 commit into from
May 17, 2023

Conversation

srlch
Copy link
Contributor

@srlch srlch commented May 16, 2023

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Which issues of this PR fixes:

Fixes #23473

Problem Summary(Required):

Problem:
Time data type is not support in storage layer, it just a data type of parameter or return value for some function.
It can not be specified by user in DDL

Solution:
check time type in DDL

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr will affect users' behaviors
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto backported to target branch
    • 3.0
    • 2.5
    • 2.4
    • 2.3

@srlch srlch requested a review from a team as a code owner May 16, 2023 08:40
@mergify mergify bot assigned srlch May 16, 2023
@imay
Copy link
Contributor

imay commented May 16, 2023

You need to check the time type when creating tables not in syntax file.

@@ -2365,7 +2365,6 @@ baseType
| DOUBLE
| DATE
| DATETIME
| TIME
| CHAR typeParameter?
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, cast(value as time) is supported. Maybe add check in DDL is better?
mysql> select cast(100 as time);
+-------------------+
| CAST(100 AS TIME) |
+-------------------+
| 00:01:00 |
+-------------------+
1 row in set (0.01 sec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Problem:
Time data type is not support in storage layer, it just a data type of parameter or return value for some function.
It can not be specified by user in DDL

Solution:
check time type in DDL

Signed-off-by: srlch <[email protected]>
@srlch srlch changed the title [BugFix] remove time data type in g4 (#23473) [BugFix] check time type in DDL (#23473) May 16, 2023
@srlch
Copy link
Contributor Author

srlch commented May 16, 2023

You need to check the time type when creating tables not in syntax file.

have fix it

@sonarcloud
Copy link

sonarcloud bot commented May 16, 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 4 Code Smells

0.0% 0.0% Coverage
57.1% 57.1% Duplication

@wanpengfei-git
Copy link
Collaborator

[FE PR Coverage Check]

😍 pass : 4 / 4 (100.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/sql/analyzer/AlterTableStatementAnalyzer.java 4 4 100.00% []

@Test
public void testAlterWithTimeType() {
analyzeFail("alter table t0 add column testcol TIME");
analyzeFail("alter table t0 modify column v0 TIME");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add a create table test case to cover creating table with TIME type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create table test case have existed in the code. We just miss for DDL

@chaoyli chaoyli merged commit e5b8a48 into StarRocks:main May 17, 2023
@wanpengfei-git
Copy link
Collaborator

@Mergifyio backport branch-3.0

@github-actions github-actions bot removed the 3.0 label May 17, 2023
@wanpengfei-git
Copy link
Collaborator

@Mergifyio backport branch-2.5

@github-actions github-actions bot removed the 2.5 label May 17, 2023
@mergify
Copy link
Contributor

mergify bot commented May 17, 2023

backport branch-3.0

✅ Backports have been created

@mergify
Copy link
Contributor

mergify bot commented May 17, 2023

backport branch-2.5

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 17, 2023
Problem:
The time data type is not supported in the storage layer, it is just a data type of
parameter or return value for some function.
It can not be specified by the user in DDL

Solution:
check time type in DDL

(cherry picked from commit e5b8a48)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/sql/analyzer/AlterTableStatementAnalyzer.java
#	fe/fe-core/src/test/java/com/starrocks/sql/analyzer/AnalyzeAlterTableStatementTest.java
mergify bot pushed a commit that referenced this pull request May 17, 2023
Problem:
The time data type is not supported in the storage layer, it is just a data type of
parameter or return value for some function.
It can not be specified by the user in DDL

Solution:
check time type in DDL

(cherry picked from commit e5b8a48)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/sql/analyzer/AlterTableStatementAnalyzer.java
#	fe/fe-core/src/test/java/com/starrocks/sql/analyzer/AnalyzeAlterTableStatementTest.java
Moonm3n pushed a commit to Moonm3n/starrocks that referenced this pull request May 23, 2023
Problem:
The time data type is not supported in the storage layer, it is just a data type of
parameter or return value for some function.
It can not be specified by the user in DDL

Solution:
check time type in DDL

Signed-off-by: Moonm3n <[email protected]>
numbernumberone pushed a commit to numbernumberone/starrocks that referenced this pull request May 31, 2023
Problem:
The time data type is not supported in the storage layer, it is just a data type of
parameter or return value for some function.
It can not be specified by the user in DDL

Solution:
check time type in DDL
numbernumberone pushed a commit to numbernumberone/starrocks that referenced this pull request May 31, 2023
Problem:
The time data type is not supported in the storage layer, it is just a data type of
parameter or return value for some function.
It can not be specified by the user in DDL

Solution:
check time type in DDL
abc982627271 pushed a commit to abc982627271/starrocks that referenced this pull request Jun 5, 2023
Problem:
The time data type is not supported in the storage layer, it is just a data type of
parameter or return value for some function.
It can not be specified by the user in DDL

Solution:
check time type in DDL
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.

[BugFix] check time type in DDL
7 participants