add SQL support for tinyint and all unsigned INTs by kmitchener · Pull Request #3309 · apache/datafusion

kmitchener · 2022-08-31T16:41:56Z

Which issue does this PR close?

Closes#3307 and closes#3325.

Rationale for this change

What changes are included in this PR?

Per the issue, this adds the following SQL datatype support:

TINYINT
UNSIGNED {TINYINT, SMALLINT, INT, BIGINT}

so now you can use this in create external table (c1 tinyint, c2 bigint unsigned) ... etc. Also available in cast() SQL:

select1::smallint unsigned;
select1::tinyint;
-- etc

Are there any user-facing changes?

remove make_data_type function in favor of convert_simple_data_type function because that one is pub modify tests to use tinyint add test for tinyint

kmitchener · 2022-08-31T16:45:24Z

@alamb @andygrove what do you think about adding unsigned int support per #3307 here vs. a separate PR? Also, in general is unsigned int support via SQL desired?

Also, note that I removed what was almost a duplicate function in planner.rs -- now it will use the same function to convert SQL datatypes to Arrow datatypes for both SQL CAST() function and for the schema that can be specified for CREATE EXTERNAL TABLE. I'd just like to verify with someone more knowledgeable than me that this is a good change.

codecov-commenter · 2022-08-31T17:19:07Z

Codecov Report

Merging #3309 (ee801bd) into master (312a7dd) will increase coverage by 0.04%.
The diff coverage is 97.67%.

@@ Coverage Diff @@## master #3309 +/- ##
==========================================
+ Coverage 85.74% 85.79% +0.04% 
==========================================
Files 294 295 +1 Lines 53749 53768 +19 ==========================================
+ Hits 46089 46132 +43 + Misses 7660 7636 -24

Impacted Files	Coverage Δ
datafusion/core/tests/sql/mod.rs	`97.77% <ø> (ø)`
datafusion/sql/src/planner.rs	`81.74% <88.88%> (+1.11%)`	⬆️
datafusion/core/tests/sql/cast.rs	`100.00% <100.00%> (ø)`
datafusion/core/tests/sql/explain_analyze.rs	`83.39% <100.00%> (ø)`
datafusion/expr/src/logical_plan/plan.rs	`77.88% <0.00%> (-0.50%)`	⬇️
datafusion/expr/src/utils.rs	`90.74% <0.00%> (-0.37%)`	⬇️
datafusion/optimizer/src/projection_push_down.rs	`98.06% <0.00%> (ø)`
...fusion/optimizer/src/single_distinct_to_groupby.rs	`98.22% <0.00%> (ø)`
...usion/optimizer/src/decorrelate_scalar_subquery.rs
...atafusion/optimizer/src/scalar_subquery_to_join.rs	`93.07% <0.00%> (ø)`
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

alamb

This looks like a really nice PR @kmitchener -- thank you

alamb · 2022-08-31T18:54:16Z

 CREATE EXTERNAL TABLE aggregate_test_100 (
 c1 VARCHAR NOT NULL,
- c2 INT NOT NULL,
+ c2 TINYINT NOT NULL,


for anyone else wondering, c8 is still a INT column

alamb · 2022-08-31T18:54:50Z

- SQLDataType::Binary(_) => Ok(DataType::Binary),
 SQLDataType::Bytea => Ok(DataType::Binary),
- other => Err(DataFusionError::NotImplemented(format!(
+ // Explicitly list all other types so that if sqlparser


this is a nice refactor to remove duplication

kmitchener · 2022-08-31T19:51:08Z

I've added the unsigned ints to this PR as well, matching what's in the issue

alamb

Looks great to me -- thank you @kmitchener

alamb · 2022-08-31T23:29:41Z

-| `REAL` | `Float32` |
-| `DOUBLE` | `Float64` |
-| `DECIMAL(p,s)` | `Decimal128(p,s)` |
+| SQL DataType | Arrow DataType |


👌 Thank you

liukun4515 · 2022-09-01T03:58:44Z

Please hold this pr, I want to review it and has concern about the data type supported in the SQL level.
@alamb @kmitchener

liukun4515 · 2022-09-01T04:03:33Z

I think the signed value is ok for me like small int, but the unsigned value make me concern about the compatibility and consistent behavior with other database system like spark https://spark.apache.org/docs/latest/sql-ref-datatypes.html#data-types

liukun4515 · 2022-09-01T04:13:14Z

I've added the unsigned ints to this PR as well, matching what's in the issue

I have the opposite opinion for supporting the unsigned value in SQL Level now.

You can fill a new issue to discuss about this, I think this is big change for API and interface which is not compatible with other database or SQL system.

alamb · 2022-09-01T12:05:55Z

Hi @liukun4515 -- thank you for the review. I will mark this PR as a draft while we discuss.

So that I can file the correct issue for discussion, can you help me understand what the concern is? Do you think:

DataFusion should not support UNSIGNED INT
DataFusion should treat SQL UNSIGNED INT as Int64
Something else?

Thanks!

kmitchener · 2022-09-01T12:12:40Z

I've added the unsigned ints to this PR as well, matching what's in the issue
I have the opposite opinion for supporting the unsigned value in SQL Level now.
You can fill a new issue to discuss about this, I think this is big change for API and interface which is not compatible with other database or SQL system.

Sure thing -- it's in #3325.

* rename DecorrelateScalarSubquery to ScalarSubqueryToJoin * cargo fmt * fix name and ref

* Add Aggregate::try_new with validation checks * fix calculation of number of grouping expressions * use suggested error message

* MINOR: add github action trigger Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * Update .github/workflows/rust.yml Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

…ions (apache#3320) * add inlist test for optimizer rule PreCastLitInComparisonExpressions * fmt code

…pache#3336) * Update signature for Expr.name so that schema is no longer required * remove some duplicate code * clippy

…che#3298)

* update both year and month Signed-off-by: remzi <13716567376yh@gmail.com> * fmt Signed-off-by: remzi <13716567376yh@gmail.com> Signed-off-by: remzi <13716567376yh@gmail.com>

* impl binary ops between binary arrays and scalars * impl binary ops between binary arrays and scalars * run fmt * add binary array input support in digest functions * remove wrong input typecheck for digest functions * fix digest function signatures * add test for binary equality * fix signature of trim function * run fmt * fix digest function signature, run fmt

* Add to_raw_logical_plan * rename the new func * change func name to to_unoptimized_plan

* add helper function for pop_expr * remove unwraps

…apache#3348) * MINOR: add df.to_unoptimized_plan() to docs, remove erroneous comment * npx prettier

* add initial support for 'drop view' * add tests * add very basic docs * add example to drop view doc * ran prettier on the doc * refactored, now done * removed typo * fix bad resolve merge conflict

…do (apache#3276)

…#3282) * Add failing union_all_on_projection test * Update projection step to replace both qualified and unqualified columns * Remove redundant clone

Bumps [lz4-sys](https://github.com/10xGenomics/lz4-rs) from 1.9.3 to 1.9.4. - [Release notes](https://github.com/10xGenomics/lz4-rs/releases) - [Changelog](https://github.com/10XGenomics/lz4-rs/blob/master/CHANGELOG.md) - [Commits](https://github.com/10xGenomics/lz4-rs/commits) --- updated-dependencies: - dependency-name: lz4-sys dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* [MINOR] Add debug logging to plan teardown * clippy

…pache#3339)

* add binary support Signed-off-by: remzi <13716567376yh@gmail.com> * support inset and add tests Signed-off-by: remzi <13716567376yh@gmail.com> * clean Signed-off-by: remzi <13716567376yh@gmail.com> * fmt Signed-off-by: remzi <13716567376yh@gmail.com> Signed-off-by: remzi <13716567376yh@gmail.com>

kmitchener · 2022-09-04T14:20:19Z

I've really messed up this branch. Going to come back to this when brain works.

add tinyint support
f6d28c6
remove make_data_type function in favor of convert_simple_data_type function because that one is pub modify tests to use tinyint add test for tinyint

github-actionsBot added core Core DataFusion crate sql SQL Planner labels Aug 31, 2022

alamb reviewed Aug 31, 2022
View reviewed changes

added the unsigned int variants and tests
ad9dff2

kmitchener marked this pull request as ready for review August 31, 2022 19:51

kmitchener changed the title ~~add tinyint SQL support~~add SQL support for tinyint and all unsigned INTs Aug 31, 2022

docs for unsigned ints
ee801bd

kmitchener requested a review from alamb August 31, 2022 20:00

alamb approved these changes Aug 31, 2022
View reviewed changes

alamb marked this pull request as draft September 1, 2022 12:06

kmitchener mentioned this pull request Sep 1, 2022
add SQL support for unsigned integers #3325
Closed

andygrove reviewed Sep 3, 2022
View reviewed changes

Comment threaddatafusion/core/tests/sql/cast.rs Outdated

kmitchenerand others added 9 commits September 4, 2022 09:09

improve tests
65bf4ec

MINOR: rename optimizer rule to ScalarSubqueryToJoin (apache#3306)
9eddbc3
* rename DecorrelateScalarSubquery to ScalarSubqueryToJoin * cargo fmt * fix name and ref

AddAggregate::try newwith validation checks (apache#3286)
b15bd77
* Add Aggregate::try_new with validation checks * fix calculation of number of grouping expressions * use suggested error message

finish integrating isTrue and similar expressions (apache#3301)
2266940

ignore failing test (apache#3329)
8d666a5

add explain sql test for optimizer rule PreCastLitInComparisonExpress…
a53354f
…ions (apache#3320) * add inlist test for optimizer rule PreCastLitInComparisonExpressions * fmt code

Update signature for Expr.name so that schema is no longer required (a…
d50176a
…pache#3336) * Update signature for Expr.name so that schema is no longer required * remove some duplicate code * clippy

Add top-level Like, ILike, SimilarTo expressions in logical plan (apa…
2f83142
…che#3298)

HaoYang670and others added 17 commits September 4, 2022 10:04

Fix the testtimestamp_add_interval_months(apache#3337)
92771a5
* update both year and month Signed-off-by: remzi <13716567376yh@gmail.com> * fmt Signed-off-by: remzi <13716567376yh@gmail.com> Signed-off-by: remzi <13716567376yh@gmail.com>

Add to_unoptimized_plan (apache#3344)
7d2da07
* Add to_raw_logical_plan * rename the new func * change func name to to_unoptimized_plan

Remove unreachable from filter push down rule (apache#3342)
9ab7377

Remove panics from simplify_expressions optimizer rule (apache#3343)
312b95e

Remove Result.unwrap from single_distinct_to_groupby (apache#3345)
aa50266

Remove panics fromcommon_subexpr_eliminate(apache#3346)
6cd1ac0
* add helper function for pop_expr * remove unwraps

MINOR: add df.to_unoptimized_plan() to docs, remove erroneous comment (…
b45e10a
…apache#3348) * MINOR: add df.to_unoptimized_plan() to docs, remove erroneous comment * npx prettier

implementdrop view(apache#3267)
e0610ec
* add initial support for 'drop view' * add tests * add very basic docs * add example to drop view doc * ran prettier on the doc * refactored, now done * removed typo * fix bad resolve merge conflict

Deprecate legacy datafusion::logical_plan module (apache#3338)
6142641

Return from task execution if send fails as there is nothing more to …
730bcd3
…do (apache#3276)

Fix SchemaError in FilterPushDown optimization with UNION ALL (apache…
8a3e114
…#3282) * Add failing union_all_on_projection test * Update projection step to replace both qualified and unqualified columns * Remove redundant clone

[MINOR] Add debug logging to plan teardown (apache#3350)
7208474
* [MINOR] Add debug logging to plan teardown * clippy

Re-implement ExprIdentifierVisitor::desc_expr to use Expr::Display (a…
dab5266
…pache#3339)

update test to reflect c2 change to Int8
134aeea

github-actionsBot added logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates labels Sep 4, 2022

kmitchener closed this Sep 5, 2022

kmitchener deleted the 3307-support-tinyint-and-friends branch September 5, 2022 12:02

kmitchener mentioned this pull request Sep 5, 2022
Add SQL support for tinyint , smallint, and unsigned int variants#3359
Merged

Uh oh!

Conversation

kmitchener commented Aug 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

kmitchener commented Aug 31, 2022

Uh oh!

codecov-commenter commented Aug 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambAug 31, 2022

Choose a reason for hiding this comment

Uh oh!

alambAug 31, 2022

Choose a reason for hiding this comment

Uh oh!

kmitchener commented Aug 31, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambAug 31, 2022

Choose a reason for hiding this comment

Uh oh!

liukun4515 commented Sep 1, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liukun4515 commented Sep 1, 2022

Uh oh!

liukun4515 commented Sep 1, 2022

Uh oh!

alamb commented Sep 1, 2022

Uh oh!

kmitchener commented Sep 1, 2022

Uh oh!

Uh oh!

kmitchener commented Sep 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

kmitchener commented Aug 31, 2022•
edited
Loading

codecov-commenter commented Aug 31, 2022•
edited
Loading

liukun4515 commented Sep 1, 2022•
edited
Loading