Add support for Utf8View for date/temporal codepaths #11518

a10y · 2024-07-17T15:34:42Z

(targets string-view2)

Which issue does this PR close?

More Utf8View support, per #10918

What changes are included in this PR?

Implements Utf8View support for folllowing:

Coercion between Utf8View and other string-y types
Coercion between Utf8View and temporal types
Sending a Utf8View into the date_part function
Sending a Utf8View into the make_date function

Are these changes tested?

Currently just using existing tests, I've tested this on the https://github.com/spiraldb/vortex/ TPC-H benchmarks.

Are there any user-facing changes?

Makes several things that used to be errors not error.

a10y · 2024-07-17T15:37:37Z

datafusion/functions/src/unicode/substr.rs

+// TODO(aduffy): just return a manipulated Utf8View array by modifying the view only.
+pub fn substr_view(args: &[ArrayRef]) -> Result<ArrayRef> {
+    match args.len() {


I'm going to revert this since it's outside the scope of this PR.

Ideally, we'd be able to defer to the substring kernel in arrow_string, but that kernel needs to be fixed to support utf8views

a10y · 2024-07-17T15:39:26Z

I see @XiangpengHao that you've got some of this in #11517, I can rebase this after your PR merges

alamb · 2024-07-18T19:59:48Z

I see @XiangpengHao that you've got some of this in #11517, I can rebase this after your PR merges

Marking as draft as I think this is waiting on some other PRs

alamb · 2024-07-18T20:52:16Z

Thanks @a10y

Also, it might help to target this PR to the string-view2 branch where we are going to accumulate various StringView code in DataFusion that might depend on features that are not yet released in arrow-rs (will be released in arrow-rs 52.2.0 soon)

alamb

I actually don't see too much replication in this PR -- I think we could target it at the string-view2 branch and it would be good to go

Thank you @a10y (and 👋 )

a10y · 2024-07-18T21:11:03Z

Hey! Just changing the base branch in the PR made a massive diff, but did some quick git surgery to cp these changes to be on top of existing string-view2 branch instead

alamb

Thanks @a10y

I think the only thing missing from this PR are some tests -- maybe we can adapt some existing sqllogictests and use StringView rather Strings 🤔

alamb · 2024-07-18T21:13:12Z

datafusion/common/src/hash_utils.rs

@@ -369,6 +369,7 @@ pub fn create_hashes<'a>(
            DataType::Null => hash_null(random_state, hashes_buffer, rehash),
            DataType::Boolean => hash_array(as_boolean_array(array)?, random_state, hashes_buffer, rehash),
            DataType::Utf8 => hash_array(as_string_array(array)?, random_state, hashes_buffer, rehash),
+            DataType::Utf8View => hash_array(as_string_view_array(array)?, random_state, hashes_buffer, rehash),


this might be the only thing that is already present

a10y · 2024-07-18T22:29:49Z

Hey @alamb, I added some sqllogictest cases. However, they will fail currently, I'd forgotten this change is dependent on the fixes in apache/arrow-rs#6077.

I was doing testing over in spiral bringing in both of these patchsets, and had not tested this one in isolation.

alamb · 2024-07-19T10:08:12Z

Hey @alamb, I added some sqllogictest cases. However, they will fail currently, I'd forgotten this change is dependent on the fixes in apache/arrow-rs#6077.

I was doing testing over in spiral bringing in both of these patchsets, and had not tested this one in isolation.

No worries -- I'll try and get apache/arrow-rs#6077 reviewed and merged today

alamb · 2024-07-19T19:19:35Z

Ok, sorry for the delay -- I just merged apache/arrow-rs#6077

I think if you updated the pin to Cargo.toml here

https://github.com/apache/datafusion/blob/string-view2/Cargo.toml
https://github.com/apache/datafusion/blob/string-view2/datafusion-cli/Cargo.toml

To be the appropriate commit in arrow-rs we could get the PR to pass

a10y · 2024-07-20T15:40:05Z

Just rebased to get #11517 changes and to bump arrow-rs patch version. sqllogictests are passing for me locally now!

alamb · 2024-07-22T13:29:24Z

Thank you very much @a10y

I took the liberty of pushing some commits to this branch to get CI to pass. Also I think by doing so the CI will run automatically for this PR from now on (and will run automatically for you in any subsequent PRs)

alamb · 2024-07-22T17:42:21Z

🚀

… some ClickBench queries (not on by default) (#11667) * Pin to pre-release version of arrow 52.2.0 * Update for deprecated method * Add a config to force using string view in benchmark (#11514) * add a knob to force string view in benchmark * fix sql logic test * update doc * fix ci * fix ci only test * Update benchmarks/src/util/options.rs Co-authored-by: Andrew Lamb <[email protected]> * Update datafusion/common/src/config.rs Co-authored-by: Andrew Lamb <[email protected]> * update tests --------- Co-authored-by: Andrew Lamb <[email protected]> * Add String view helper functions (#11517) * add functions * add tests for hash util * Add ArrowBytesViewMap and ArrowBytesViewSet (#11515) * Update `string-view` branch to arrow-rs main (#10966) * Pin to arrow main * Fix clippy with latest arrow * Uncomment test that needs new arrow-rs to work * Update datafusion-cli Cargo.lock * Update Cargo.lock * tapelo * merge * update cast * consistent dep * fix ci * add more tests * make doc happy * update new implementation * fix bug * avoid unused dep * update dep * update * fix cargo check * update doc * pick up the comments change again --------- Co-authored-by: Andrew Lamb <[email protected]> * Enable `GroupValueBytesView` for aggregation with StringView types (#11519) * add functions * Update `string-view` branch to arrow-rs main (#10966) * Pin to arrow main * Fix clippy with latest arrow * Uncomment test that needs new arrow-rs to work * Update datafusion-cli Cargo.lock * Update Cargo.lock * tapelo * merge * update cast * consistent dep * fix ci * avoid unused dep * update dep * update * fix cargo check * better group value view aggregation * update --------- Co-authored-by: Andrew Lamb <[email protected]> * Initial support for regex_replace on `StringViewArray` (#11556) * initial support for string view regex * update tests * Add support for Utf8View for date/temporal codepaths (#11518) * Add StringView support for date_part and make_date funcs * run cargo update in datafusion-cli * cargo fmt --------- Co-authored-by: Andrew Lamb <[email protected]> * GC `StringViewArray` in `CoalesceBatchesStream` (#11587) * gc string view when appropriate * make clippy happy * address comments * make doc happy * update style * Add comments and tests for gc_string_view_batch * better herustic * update test * Update datafusion/physical-plan/src/coalesce_batches.rs Co-authored-by: Andrew Lamb <[email protected]> --------- Co-authored-by: Andrew Lamb <[email protected]> * [Bug] fix bug in return type inference of `utf8_to_int_type` (#11662) * fix bug in return type inference * update doc * add tests --------- Co-authored-by: Andrew Lamb <[email protected]> * Fix clippy * Increase ByteViewMap block size to 2MB (#11674) * better default block size * fix related test * Change `--string-view` to only apply to parquet formats (#11663) * use inferenced schema, don't load schema again * move config to parquet-only * update * update * better format * format * update * Implement native support StringView for character length (#11676) * native support for character length * Update datafusion/functions/src/unicode/character_length.rs --------- Co-authored-by: Andrew Lamb <[email protected]> * Remove uneeded patches * cargo fmt --------- Co-authored-by: Xiangpeng Hao <[email protected]> Co-authored-by: Xiangpeng Hao <[email protected]> Co-authored-by: Andrew Duffy <[email protected]>

github-actions bot added the logical-expr Logical plan and expressions label Jul 17, 2024

a10y changed the title ~~Fixes to incorporate stringview~~ Add support for Utf8View for date/temporal codepaths Jul 17, 2024

a10y commented Jul 17, 2024

View reviewed changes

a10y mentioned this pull request Jul 17, 2024

[blocked] Switch to Utf8View for TPC-H spiraldb/vortex#476

Closed

alamb marked this pull request as draft July 18, 2024 19:59

alamb reviewed Jul 18, 2024

View reviewed changes

a10y changed the base branch from main to string-view2 July 18, 2024 21:04

a10y changed the base branch from string-view2 to main July 18, 2024 21:05

a10y force-pushed the aduffy/utf8view branch from 2bc8b54 to 7ddd7e6 Compare July 18, 2024 21:09

github-actions bot added the core Core DataFusion crate label Jul 18, 2024

a10y changed the base branch from main to string-view2 July 18, 2024 21:10

alamb reviewed Jul 18, 2024

View reviewed changes

github-actions bot added sqllogictest SQL Logic Tests (.slt) and removed core Core DataFusion crate labels Jul 18, 2024

Add StringView support for date_part and make_date funcs

580e64b

a10y force-pushed the aduffy/utf8view branch from deb7351 to 580e64b Compare July 20, 2024 15:38

a10y marked this pull request as ready for review July 20, 2024 15:38

alamb added 2 commits July 22, 2024 09:28

run cargo update in datafusion-cli

9561e07

cargo fmt

f8a7e24

alamb merged commit bb780b3 into apache:string-view2 Jul 22, 2024
25 checks passed

a10y deleted the aduffy/utf8view branch July 22, 2024 20:19

alamb mentioned this pull request Jul 26, 2024

Merge string-view2 branch: reading from parquet up to 2x faster for some ClickBench queries (not on by default) #11667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Utf8View for date/temporal codepaths #11518

Add support for Utf8View for date/temporal codepaths #11518

a10y commented Jul 17, 2024 •

edited by alamb

Loading

a10y Jul 17, 2024

a10y commented Jul 17, 2024

alamb commented Jul 18, 2024

alamb commented Jul 18, 2024

alamb left a comment

a10y commented Jul 18, 2024

alamb left a comment

alamb Jul 18, 2024

a10y commented Jul 18, 2024

alamb commented Jul 19, 2024

alamb commented Jul 19, 2024

a10y commented Jul 20, 2024

alamb commented Jul 22, 2024

alamb commented Jul 22, 2024

Add support for Utf8View for date/temporal codepaths #11518

Add support for Utf8View for date/temporal codepaths #11518

Conversation

a10y commented Jul 17, 2024 • edited by alamb Loading

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

a10y Jul 17, 2024

Choose a reason for hiding this comment

a10y commented Jul 17, 2024

alamb commented Jul 18, 2024

alamb commented Jul 18, 2024

alamb left a comment

Choose a reason for hiding this comment

a10y commented Jul 18, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Jul 18, 2024

Choose a reason for hiding this comment

a10y commented Jul 18, 2024

alamb commented Jul 19, 2024

alamb commented Jul 19, 2024

a10y commented Jul 20, 2024

alamb commented Jul 22, 2024

alamb commented Jul 22, 2024

a10y commented Jul 17, 2024 •

edited by alamb

Loading