Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jun 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
227fc39
feat: Implement equality = and inequality <> support for StringView
Weijun-H Jun 18, 2024
9a9006c
chore: Add tests for the StringView
Weijun-H Jun 18, 2024
5930faa
chore
Weijun-H Jun 18, 2024
e36a93e
chore: Update tests for NULL
Weijun-H Jun 18, 2024
0f0de6d
fix: Used build_array_string!
Weijun-H Jun 19, 2024
6659d04
chore: Update string_coercion function to handle Utf8View type in bin…
Weijun-H Jun 19, 2024
308075d
chore: add tests
Weijun-H Jun 19, 2024
94ede95
chore: ci
Weijun-H Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,15 @@ unused_imports = "deny"
## Temporary arrow-rs patch until 52.1.0 is released

[patch.crates-io]
arrow = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-array = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-buffer = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-cast = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-data = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-ipc = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-schema = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-select = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-string = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-ord = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-flight = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
parquet = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-array = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-buffer = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-cast = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-data = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-ipc = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-schema = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-select = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-string = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-ord = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-flight = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
parquet = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
30 changes: 15 additions & 15 deletions datafusion-cli/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 11 additions & 11 deletions datafusion-cli/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,14 @@ rstest = "0.17"
## Temporary arrow-rs patch until 52.1.0 is released

[patch.crates-io]
arrow = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-array = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-buffer = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-cast = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-data = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-ipc = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-schema = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-select = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-string = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow-ord = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
parquet = { git = "https://github.com/apache/arrow-rs.git", rev = "72467c670f8c38130e4743347407f1a542e59e0c" }
arrow = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-array = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-buffer = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-cast = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-data = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-ipc = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-schema = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-select = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-string = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
arrow-ord = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
parquet = { git = "https://github.com/apache/arrow-rs.git", rev = "d0a88c651991b7fc4b970cf94fa77f4ec3def22d" }
17 changes: 16 additions & 1 deletion datafusion/common/src/scalar/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1715,6 +1715,22 @@ impl ScalarValue {
)?;
Arc::new(array)
}
DataType::Utf8View => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

let array = scalars
.map(|sv| {
if let ScalarValue::Utf8View(v) = sv {
Ok(v)
} else {
_internal_err!(
"Inconsistent types in ScalarValue::iter_to_array. \
Expected {data_type:?}, got {sv:?}"
)
}
})
.collect::<Result<Vec<_>>>()?;
let array = StringViewArray::from(array);
Arc::new(array)
}
// explicitly enumerate unsupported types so newly added
// types must be aknowledged, Time32 and Time64 types are
// not supported if the TimeUnit is not valid (Time32 can
Expand All @@ -1726,7 +1742,6 @@ impl ScalarValue {
| DataType::Time64(TimeUnit::Millisecond)
| DataType::Map(_, _)
| DataType::RunEndEncoded(_, _)
| DataType::Utf8View
| DataType::BinaryView
| DataType::ListView(_)
| DataType::LargeListView(_) => {
Expand Down
1 change: 1 addition & 0 deletions datafusion/expr/src/type_coercion/binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

_ => None,
}
}
Expand Down
65 changes: 65 additions & 0 deletions datafusion/sqllogictest/test_files/string_view.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at

# http://www.apache.org/licenses/LICENSE-2.0

# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

statement ok
create table test as values (arrow_cast('Andrew', 'Utf8View'), arrow_cast('X', 'Utf8View')),
(arrow_cast('Xiangpeng', 'Utf8View'), arrow_cast('Xiangpeng', 'Utf8View')),
(arrow_cast('Raphael', 'Utf8View'), arrow_cast('R', 'Utf8View')),
(arrow_cast(NULL, 'Utf8View'), arrow_cast('R', 'Utf8View'));

query B
select arrow_cast('NULL', 'Utf8View') = arrow_cast('Andrew', 'Utf8View');
----
false

query B
select arrow_cast('NULL', 'Utf8View') <> arrow_cast('Andrew', 'Utf8View');
----
true

query B
select arrow_cast('Andrew', 'Utf8View') = arrow_cast('Andrew', 'Utf8View');
----
true

query B
select arrow_cast('Xiangpeng', 'Utf8View') <> arrow_cast('Andrew', 'Utf8View');
----
true

query ??
select * from test where column1 = column2;
----
Xiangpeng Xiangpeng

query ??
select * from test where column1 <> column2;
----
Andrew X
Raphael R


query ??
select * from test where column1 = arrow_cast('Andrew', 'Utf8View');
----
Andrew X

query ??
select * from test where column1 <> arrow_cast('Andrew', 'Utf8View');
----
Xiangpeng Xiangpeng
Raphael R