-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support INTEGER
again in addition to INT
in CREATE TABLE
and CAST
statements
#3167
Conversation
cc @waitingkuo |
@@ -631,7 +631,7 @@ async fn register_aggregate_csv_by_sql(ctx: &SessionContext) { | |||
c2 INT NOT NULL, | |||
c3 SMALLINT NOT NULL, | |||
c4 SMALLINT NOT NULL, | |||
c5 INT NOT NULL, | |||
c5 INTEGER NOT 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.
with this change, many of the sql_integration
tests fail
@@ -483,7 +483,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
fn make_data_type(&self, sql_type: &SQLDataType) -> Result<DataType> { | |||
match sql_type { | |||
SQLDataType::BigInt(_) => Ok(DataType::Int64), | |||
SQLDataType::Int(_) => Ok(DataType::Int32), | |||
SQLDataType::Int(_) | SQLDataType::Integer(_) => Ok(DataType::Int32), |
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.
this is the fix
@@ -498,7 +498,30 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { | |||
SQLDataType::Date => Ok(DataType::Date32), | |||
SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Nanosecond)), | |||
SQLDataType::Timestamp => Ok(DataType::Timestamp(TimeUnit::Nanosecond, None)), | |||
_ => Err(DataFusionError::NotImplemented(format!( | |||
// Explicitly list all other types so that if sqlparser |
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.
This does not change the behavior, but makes the current behavior more explicit in my opinion
It also makes clear some surprising things (like several unsigned
variants appear not to be supported along with interval
as noted by @waitingkuo in #3166 (comment)
INTEGER
again in addition to INT
INTEGER
again in addition to INT
in CREATE TABLE
and CAST
statements
LGTM. thank you @alamb |
Codecov Report
@@ Coverage Diff @@
## master #3167 +/- ##
==========================================
- Coverage 85.87% 85.85% -0.03%
==========================================
Files 291 291
Lines 52758 52778 +20
==========================================
+ Hits 45307 45310 +3
- Misses 7451 7468 +17
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Benchmark runs are scheduled for baseline = 8e9a8d5 and contender = 89bcfc4. 89bcfc4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3059
Rationale for this change
Fixes a regresssion --
CREATE TABLE (x INTEGER)
does not work on master, but used to previouslyapache/datafusion-sqlparser-rs#525 made
INT
andINTEGER
different types (previouslyINT
parsed toINTEGER
). However, DataFusion was not updated.What changes are included in this PR?
match
to try and avoid such regressions in the futureAre there any user-facing changes?
Bug is fixed