-
Notifications
You must be signed in to change notification settings - Fork 573
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
fix: unify varchar
format_type display with PostgreSQL
#12648
Conversation
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.
LGTM. Would you please help to fix failed e2e tests, while also adding this query to e2e_test/batch/functions/format_type.slt.part
?
SELECT format_type(1043, '-1');
Hi, should work fine now. |
src/common/src/types/mod.rs
Outdated
@@ -122,7 +122,7 @@ pub enum DataType { | |||
#[display("date")] | |||
#[from_str(regex = "(?i)^date$")] | |||
Date, | |||
#[display("varchar")] | |||
#[display("character varying")] | |||
#[from_str(regex = "(?i)^varchar$")] |
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.
Where do we depend on this? 👀
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.
Are you asking what program relies on this format type? If so, then here it it type deserialization of SqlAlchemy.
Hum, if you're asking if there are any program that already rely on this varchar
format type, I don't know 🤕.
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've confirmed that from_str
is only used in tests (like expr::build_from_pretty
), so it's not a big deal. However, what about also changing this for consistency? Take int32
as an example of how to accept both:
risingwave/src/common/src/types/mod.rs
Line 108 in 01c7c2b
#[from_str(regex = "(?i)^integer$|^int$|^int4$")] |
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.
Is this what you mean:
#[from_str(regex = "(?i)^character varying$|^varchar$")]
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.
committed it, plz review.
102db0e
to
68b2df2
Compare
Codecov Report
@@ Coverage Diff @@
## main #12648 +/- ##
==========================================
- Coverage 69.30% 69.28% -0.02%
==========================================
Files 1470 1470
Lines 241283 241286 +3
==========================================
- Hits 167212 167174 -38
- Misses 74071 74112 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Make format type display of RisingWave consistent with PostgreSQL.
The original purpose of this PR is to fix the problem that the
varchar
type in superset cannot be recognized.superset uses sqlalchemy as the ORM, and the different format type from PostgreSQL makes the type unrecognizable to sqlalchemy.
The inconsistencies are:
result of RisingWave:
varchar
; of PostgreSQL:character varying
.SqlAlchemy test:
related #9938.
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note