feat: Implement equality = and inequality <> support for StringView #10985

Weijun-H · 2024-06-18T16:29:55Z

Note: targets string-view branch

Which issue does this PR close?

Closes #10919

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

XiangpengHao

The pr looks good to me, left some minor comments

XiangpengHao · 2024-06-18T20:40:17Z

datafusion/common/src/scalar/mod.rs

@@ -1715,6 +1715,22 @@ impl ScalarValue {
                )?;
                Arc::new(array)
            }
+            DataType::Utf8View => {


Maybe just build_array_string!(StringViewArray, Utf8View)?
Can you also move this case closer to where we handle Utf8 and LargeUtf8?

XiangpengHao · 2024-06-18T20:44:08Z

datafusion/expr/src/type_coercion/binary.rs

@@ -932,6 +932,7 @@ fn string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
        (LargeUtf8, Utf8) => Some(LargeUtf8),
        (Utf8, LargeUtf8) => Some(LargeUtf8),
        (LargeUtf8, LargeUtf8) => Some(LargeUtf8),
+        (Utf8View, Utf8View) => Some(Utf8View),


Should we handle (Utf8View, Utf8), (Utf8, Utf8View) etc?
It's possible that we read a StringViewArray from parquet, and compare it against a Utf8. What do you think the result type should look like? @alamb

I agree we should permit to/coercion from any "string" / "binary" type to the associated view type.

It is probably good to make sure the coercion does the cheap direction when possible

For example, we shouldn't be converting StringViewArray --> StringArray as that will copy all the strings. Instead we should convert StringArray --> StringViewArray which is relatively cheap (we just have to copy the views)

I think this feature -- coercion -- is probably worth its own ticket and doesn't need to be done in this PR. I'll file a follow on ticket in the morning

I handled the Utf8View coercion in this PR, but it would be better to handle the BinaryView in a follow-up.

The problem with (Utf8View, Utf8) is that it can be quite common to have the following queries:

select * from t where c1 <> 'small';

Where the c1 is read to be StringViewArray, then we need to decide what is the result type (in order to implement the coercion). I agree with @alamb that StringViewArray is generally cheaper than StringArray, so we might just use Utf8View if at least one operand is using Utf8View.

I handled the Utf8View coercion in this PR, but it would be better to handle the BinaryView in a follow-up.

Filed #10996 to track Binary view support

The problem with (Utf8View, Utf8) is that it can be quite common to have the following queries:

select * from t where c1 <> 'small';

Where the c1 is read to be StringViewArray, then we need to decide what is the result type (in order to implement the coercion). I agree with @alamb that StringViewArray is generally cheaper than StringArray, so we might just use Utf8View if at least one operand is using Utf8View.

I wrote a bunch of tests in #10997 and the coercion mostly seems to work well. There is one exception which i think is minor and i will file a ticket about

alamb

Thanks @Weijun-H -- I think once @XiangpengHao 's comments are addressed this PR will be good to go. 🙏

alamb · 2024-06-19T00:03:26Z

datafusion/expr/src/type_coercion/binary.rs

@@ -932,6 +932,7 @@ fn string_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType>
        (LargeUtf8, Utf8) => Some(LargeUtf8),
        (Utf8, LargeUtf8) => Some(LargeUtf8),
        (LargeUtf8, LargeUtf8) => Some(LargeUtf8),
+        (Utf8View, Utf8View) => Some(Utf8View),


I agree we should permit to/coercion from any "string" / "binary" type to the associated view type.

It is probably good to make sure the coercion does the cheap direction when possible

For example, we shouldn't be converting StringViewArray --> StringArray as that will copy all the strings. Instead we should convert StringArray --> StringViewArray which is relatively cheap (we just have to copy the views)

I think this feature -- coercion -- is probably worth its own ticket and doesn't need to be done in this PR. I'll file a follow on ticket in the morning

…ary.rs

XiangpengHao · 2024-06-19T02:35:24Z

datafusion/sqllogictest/test_files/string_view.slt

+Andrew X
+
+query ??
+select * from test where column1 = 'Andrew';


I think for this test to run, we need to add (Utf8View, Utf8) => Utf8View, see #10985 (comment)

alamb

Thank you @XiangpengHao -- I think this pR is a good step forward and so I think we should merge it into the string-view branch to keep making progress.

I will do so and file some follow on issues to track remaining work (specifically for coercion and BinaryView)

…velopment branch (#11402) * 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 * feat: Implement equality = and inequality <> support for StringView (#10985) * feat: Implement equality = and inequality <> support for StringView * chore: Add tests for the StringView * chore * chore: Update tests for NULL * fix: Used build_array_string! * chore: Update string_coercion function to handle Utf8View type in binary.rs * chore: add tests * chore: ci * Add more StringView comparison test coverage (#10997) * Add more StringView comparison test coverage * add reference * Add another test showing casting on columns works correctly * feat: Implement equality = and inequality <> support for BinaryView (#11004) * feat: Implement equality = and inequality <> support for BinaryView Signed-off-by: Chojan Shang <[email protected]> * chore: make fmt happy Signed-off-by: Chojan Shang <[email protected]> --------- Signed-off-by: Chojan Shang <[email protected]> * Implement support for LargeString and LargeBinary for StringView and BinaryView (#11034) * implement large binary * add tests for large string * better comments for string coercion * Improve filter predicates with `Utf8View` literals (#11043) * refactor: Improve type coercion logic in TypeCoercionRewriter * refactor: Improve type coercion logic in TypeCoercionRewriter * chore * chore: Update test * refactor: Improve type coercion logic in TypeCoercionRewriter * refactor: Remove unused import and update code formatting in unwrap_cast_in_comparison.rs * Remove arrow-patch --------- Signed-off-by: Chojan Shang <[email protected]> Co-authored-by: Alex Huang <[email protected]> Co-authored-by: Chojan Shang <[email protected]> Co-authored-by: Xiangpeng Hao <[email protected]>

…velopment branch (apache#11402) * Update `string-view` branch to arrow-rs main (apache#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 * feat: Implement equality = and inequality <> support for StringView (apache#10985) * feat: Implement equality = and inequality <> support for StringView * chore: Add tests for the StringView * chore * chore: Update tests for NULL * fix: Used build_array_string! * chore: Update string_coercion function to handle Utf8View type in binary.rs * chore: add tests * chore: ci * Add more StringView comparison test coverage (apache#10997) * Add more StringView comparison test coverage * add reference * Add another test showing casting on columns works correctly * feat: Implement equality = and inequality <> support for BinaryView (apache#11004) * feat: Implement equality = and inequality <> support for BinaryView Signed-off-by: Chojan Shang <[email protected]> * chore: make fmt happy Signed-off-by: Chojan Shang <[email protected]> --------- Signed-off-by: Chojan Shang <[email protected]> * Implement support for LargeString and LargeBinary for StringView and BinaryView (apache#11034) * implement large binary * add tests for large string * better comments for string coercion * Improve filter predicates with `Utf8View` literals (apache#11043) * refactor: Improve type coercion logic in TypeCoercionRewriter * refactor: Improve type coercion logic in TypeCoercionRewriter * chore * chore: Update test * refactor: Improve type coercion logic in TypeCoercionRewriter * refactor: Remove unused import and update code formatting in unwrap_cast_in_comparison.rs * Remove arrow-patch --------- Signed-off-by: Chojan Shang <[email protected]> Co-authored-by: Alex Huang <[email protected]> Co-authored-by: Chojan Shang <[email protected]> Co-authored-by: Xiangpeng Hao <[email protected]>

Weijun-H added 2 commits June 19, 2024 00:13

feat: Implement equality = and inequality <> support for StringView

227fc39

chore: Add tests for the StringView

9a9006c

github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Jun 18, 2024

Weijun-H added 2 commits June 19, 2024 00:33

chore

5930faa

chore: Update tests for NULL

e36a93e

XiangpengHao reviewed Jun 18, 2024

View reviewed changes

alamb reviewed Jun 19, 2024

View reviewed changes

Weijun-H added 3 commits June 19, 2024 09:45

fix: Used build_array_string!

0f0de6d

chore: Update string_coercion function to handle Utf8View type in bin…

6659d04

…ary.rs

chore: add tests

308075d

XiangpengHao reviewed Jun 19, 2024

View reviewed changes

chore: ci

94ede95

Weijun-H requested review from XiangpengHao and alamb June 19, 2024 03:12

alamb approved these changes Jun 19, 2024

View reviewed changes

alamb mentioned this pull request Jun 19, 2024

Implement equality = and inequality <> support for BinaryView #10996

Closed

alamb merged commit 507d978 into apache:string-view Jun 19, 2024
26 checks passed

This was referenced Jun 19, 2024

Implement equality = and inequality <> support for StringView #10919

Closed

Add more StringView comparison test coverage #10997

Merged

Support for LargeString and LargeBinary for StringView and BinaryView #11023

Closed

alamb mentioned this pull request Jul 10, 2024

Initial support for StringView, merge changes from string-view development branch #11402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement equality = and inequality <> support for StringView #10985

feat: Implement equality = and inequality <> support for StringView #10985

Weijun-H commented Jun 18, 2024 •

edited by alamb

Loading

XiangpengHao left a comment

XiangpengHao Jun 18, 2024

XiangpengHao Jun 18, 2024

alamb Jun 19, 2024

Weijun-H Jun 19, 2024

XiangpengHao Jun 19, 2024

alamb Jun 19, 2024

alamb Jun 19, 2024

alamb left a comment

alamb Jun 19, 2024

XiangpengHao Jun 19, 2024

alamb left a comment

feat: Implement equality = and inequality <> support for StringView #10985

feat: Implement equality = and inequality <> support for StringView #10985

Conversation

Weijun-H commented Jun 18, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

XiangpengHao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Weijun-H commented Jun 18, 2024 •

edited by alamb

Loading