Skip to content

Issue 115 Arrow support: replace uints by ints in resulted RecordBatch#287

Merged
YaroslavLitvinov merged 3 commits intomainfrom
issue/115_arrow_support_replace_unisgned_by_signed
Feb 20, 2025
Merged

Issue 115 Arrow support: replace uints by ints in resulted RecordBatch#287
YaroslavLitvinov merged 3 commits intomainfrom
issue/115_arrow_support_replace_unisgned_by_signed

Conversation

@YaroslavLitvinov
Copy link
Contributor

@YaroslavLitvinov YaroslavLitvinov commented Feb 20, 2025 •
edited
Loading

Closes #115

No impact on result when json format enabled.
When output arrow format is enabled it map all the unsigned int types to signed int types in result batch:
UInt64 -> Decimal128(38, 10)
UInt32 -> Int64
UInt16 -> Int32
UInt8 -> Int16

Tested with dbt, and b using following query:
#115 (comment)

@YaroslavLitvinov YaroslavLitvinov changed the title replace uints by ints in resulted RecordBatch Issue 115 Arrow support: replace uints by ints in resulted RecordBatch Feb 20, 2025
@YaroslavLitvinov YaroslavLitvinov self-assigned this Feb 20, 2025
Vedin
Vedin requested changes Feb 20, 2025
View reviewed changes
DanCodedThis
DanCodedThis approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@DanCodedThis DanCodedThis left a comment

Choose a reason for hiding this comment

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

LGTM, only concern is what Denys mentioned

@YaroslavLitvinov
Copy link
Contributor Author

YaroslavLitvinov commented Feb 20, 2025 •
edited
Loading

I tried to replace DataType::Decimal128(38, 10) by DataType::Decimal128(38, 0) that is used with Decimal128Array and I failed. I'm unable to create array of type: PrimitiveArray<Decimal128(38, 0)>.
Surprisingly Decimal128Array hardly ties on Decimal128(38, 10) datatype, and thus it's creating arrays of kind PrimitiveArray<Decimal128(38, 10)>

If my thoughts are correct, in order to work with arrays like PrimitiveArray<Decimal128(38, 0)> it's not enough just putting scale=0, to get rid of fractional part. As it gets to compile errors that is not a data type but variable/constant. And it require creating own datatype with implementing/copy-pasting all the traits for pub struct Decimal128Type {} as it done for original Decimal128Array. And efforts for support our own datatype.
Another thought I have is about how arrow ipc will conduct witch such array type, will it encode/decode it - I don't know.
I wouldn't do such thing and would used how it is implemented now with DataType::Decimal128(38, 10)
Any thoughts on it? As I stuck with it.

@YaroslavLitvinov
Copy link
Contributor Author

YaroslavLitvinov commented Feb 20, 2025

@Vedin @osipovartem ^^

@YaroslavLitvinov
Copy link
Contributor Author

YaroslavLitvinov commented Feb 20, 2025

I tried to replace DataType::Decimal128(38, 10) by DataType::Decimal128(38, 0) that is used with Decimal128Array and I failed. I'm unable to create array of type: PrimitiveArray<Decimal128(38, 0)>. Surprisingly Decimal128Array hardly ties on Decimal128(38, 10) datatype, and thus it's creating arrays of kind PrimitiveArray<Decimal128(38, 10)>

If my thoughts are correct, in order to work with arrays like PrimitiveArray<Decimal128(38, 0)> it's not enough just putting scale=0, to get rid of fractional part. As it gets to compile errors that is not a data type but variable/constant. And it require creating own datatype with implementing/copy-pasting all the traits for pub struct Decimal128Type {} as it done for original Decimal128Array. And efforts for support our own datatype. Another thought I have is about how arrow ipc will conduct witch such array type, will it encode/decode it - I don't know. I wouldn't do such thing and would used how it is implemented now with DataType::Decimal128(38, 10) Any thoughts on it? As I stuck with it.

with_precision_and_scale helped as Vedin suggested.

@YaroslavLitvinov YaroslavLitvinov force-pushed the issue/115_arrow_support_replace_unisgned_by_signed branch from af1ccc9 to f9e5e93 Compare February 20, 2025 19:13
@YaroslavLitvinov
Copy link
Contributor Author

YaroslavLitvinov commented Feb 20, 2025

test / ubuntu / stable (pull_request)
Failing after 11m Test Error: Process completed with exit code 143.
On dev environement it stuck on:
test service::tests::test_delete_session has been running for over 60 seconds

@Vedin Vedin self-requested a review February 20, 2025 20:02
@YaroslavLitvinov YaroslavLitvinov merged commit fac1cdd into main Feb 20, 2025
4 of 5 checks passed
@YaroslavLitvinov YaroslavLitvinov deleted the issue/115_arrow_support_replace_unisgned_by_signed branch February 20, 2025 20:06
eadgbear pushed a commit that referenced this pull request Feb 27, 2025
@YaroslavLitvinov @eadgbear
Issue 115 Arrow support: replace uints by ints in resulted RecordBatch (
06abb56 
#287)

* replace uints by ints in resulted RecordBatch

* cargo clippy --workspace --tests

* rebase & fixes
@eadgbear eadgbear mentioned this pull request Feb 27, 2025
Merged
eadgbear added a commit that referenced this pull request Mar 7, 2025
@eadgbear @osipovartem
@YaroslavLitvinov @Vedin @rampage644 @DanCodedThis
(WIP) Metastore + Unified Query 1.0 (#312)
70793f5 
* Initial metastore crate

* Metastore implementation

* Upgrading Slatedb to 0.4, adding snapshot tests

* Checkpoint commit

* Checkpoint before crate re-org

* Fix timestamp displaying format (#279)

* fix timestamp format

* Revert timestamp change

* in login request use database_name instead of warehouse (#277)

* Adding missing licenses + license check (#278)

* Adding missing licenses + license check

* Fixing license config path

* Fix IPC arrow format issue (#280)

* Fix IPC arrow format issue

* Fix IPC arrow format issue

* Fix comments

* Control plane tests (batch 3) (#274)

* Add CP service tests

* Add create/delete session tests

* Add create/delete session tests

* Use converted S3 error

* Move DBT_SERIALIZATION_FORMAT to cli (#286)

* Move dbt_ser_fmt to cli

* Rename the field

* Update query runner with session id (#285)

* Issue 115 Arrow support: replace uints by ints in resulted RecordBatch (#287)

* replace uints by ints in resulted RecordBatch

* cargo clippy --workspace --tests

* rebase & fixes

* Set warehouse location based on storage profile path (#288)

* map float types to real logical_type (#289)

* map float types to real logical_type

* separate sessions tests to avoid blocking issues

* unit test for floats datatype maped into real logical type

* Snowflake timestamp_from_parts udf (#297)

* Inital version

* snowflake timestamp_from_parts udf

* Fix clippy

* Fix example typo

* Support negative values

* Snowflake time_from_parts udf (#298)

* Inital version

* snowflake timestamp_from_parts udf

* snowflake time_from_parts udf

* snowflake time_from_parts udf

* Fix clippy

* Fix clippy

* Fix example typo

* Support negative values

* Merge

* snowflake date_from_parts udf (#299)

* Inital version

* snowflake timestamp_from_parts udf

* snowflake time_from_parts udf

* snowflake time_from_parts udf

* snowflake date_from_parts udf

* Fix clippy

* Implement extraction of the corresponding date part from a date or timestamp (#302)

* Implement extracts the corresponding date part from a date or timestamp

* Add units for visit expressions

* Add geospatial udfs (#304)

* Fix object store for custom endpoint (#295)

AWS S3 builder uses dedicated setting to allow http. Parse endpoint and enable if it isn't https.

* Unified query, execution, catalog

* Adding temporary tables to metastore

* Custom DataFusion Catalog implementation (WIP)

* Adding execution User Session type

* Checkpoint

* Catalog implementation for IB

* Crate re-org complete, writing tests

* Changing metastore crate to use iceberg_rust types

* Metastore crate cleanup

* Create table query fixed

* Fixing copy_into query

* Finished updating query type

* Cleaning up service type

* Wrapping up changes to the execution crate

* Bumping Datafusion git ref

* Fixing up after package reorg

* Fixed DBT handler

* Removing old crates

* Disabling some UI handlers and models until new metastore integration is done

* Disabling extra OpenAPI endpoints and some unit tests while test are fixed

* Deleting manifest files that snuck into the repo (my bad)

* Final compilation and clippy fixes

* `cargo fix`

* `cargo fmt`

* Adding license blocks to new files

* Adding missing licenses + license check (#278)

* Adding missing licenses + license check

* Fixing license config path

* Fix IPC arrow format issue (#280)

* Fix IPC arrow format issue

* Fix IPC arrow format issue

* Fix comments

* Control plane tests (batch 3) (#274)

* Add CP service tests

* Add create/delete session tests

* Add create/delete session tests

* Use converted S3 error

* Move DBT_SERIALIZATION_FORMAT to cli (#286)

* Move dbt_ser_fmt to cli

* Rename the field

* Snowflake timestamp_from_parts udf (#297)

* Inital version

* snowflake timestamp_from_parts udf

* Fix clippy

* Fix example typo

* Support negative values

* Implement extraction of the corresponding date part from a date or timestamp (#302)

* Implement extracts the corresponding date part from a date or timestamp

* Add units for visit expressions

* Add geospatial udfs (#304)

* patch chrono 0.4.40 as it breaks build (#310)

* Added support for `day` and other datetime keywords in DF (#294)

* start here

* no macro, just simple vistor mut

* remvoe redundant trait creation

* integration test add, fixed logic, next unit tests

* activated impl and cargo fmt + clippy

* added unit tests + impl extension

* cargo fmt + clippy

* cargo clippy

* cargo clippy

* removed redundant check

* fix

* merged with Artem PR

* cargo fmt + clippy

* small fixes

* cargo fmt + clippy

* update hash commis for datafusion, iceberg-rust (#301)

* Update tests.yml (#313)

Adding github large runners to tests

* Return hardcoded is_dynamic (#318)

* Add geospatial UDF st_makeline (#315)

* Add geospatial udfs

* Add geospatial st_makeline

* Fix cargo

* Add lic header

* Change allocator to snmalloc (#317)

* Add ST_MakePolygon, st_polygon (#319)

* Add ST_MakePolygon, st_polygon

* Add docs

* Add ST_DIMENSION  (#321)

* Add ST_MakePolygon, st_polygon

* Add docs

* Add ST_dimention

* Add license

* Add ST_PointN and ST_Endpoint (#322)

* Add ST_MakePolygon, st_polygon

* Add docs

* Add ST_dimention

* Add license

* Add ST_Endpoint, ST_PointN

* Reimplement ST_Point to work with snowflake linestring

* Add St_x, st_y, st_srid (#324)

* Add ST_MakePolygon, st_polygon

* Add docs

* Add ST_dimention

* Add license

* Add ST_Endpoint, ST_PointN

* Reimplement ST_Point to work with snowflake linestring

* Add St_x, st_y, st_srid

* Moving UDF registration

* Changing Metastore to use Dashmap

* Adding missing geospatial functions after merge

* Fixing unit test structure, still need to fix some unit tests

* Post-merge cleanup

---------

Co-authored-by: Artem Osipov <59066880+osipovartem@users.noreply.github.com>
Co-authored-by: Yaroslav Litvinov <yaroslav@embucket.com>
Co-authored-by: Denys Tsomenko <denistsem@gmail.com>
Co-authored-by: Sergei Turukin <sergei@embucket.com>
Co-authored-by: DanCodedThis <94703934+DanCodedThis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@osipovartem osipovartem osipovartem approved these changes

@Vedin Vedin Vedin approved these changes

@DanCodedThis DanCodedThis DanCodedThis approved these changes

Assignees

@YaroslavLitvinov YaroslavLitvinov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

api: switch from JSON data type to ARROW IPC

4 participants

@YaroslavLitvinov @Vedin @osipovartem @DanCodedThis