You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "NickCrews (via GitHub)" <gi...@apache.org> on 2024/08/21 21:21:08 UTC
[PR] [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
NickCrews opened a new pull request, #43782:
URL: https://github.com/apache/arrow/pull/43782
Fixes https://github.com/apache/arrow/issues/33592
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1945885089
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
+ return Status::TypeError("field '", target_field->name(),
Review Comment:
Ignore me, i previously think we can
```
auto values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
if (out field not nullable but in field nullable) {
if (values->GetNullCount() > 0) {
return error
}
values->buffers[0].reset();
}
```
But I also think this doesn't matter here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1891412003
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
> I have never heard of kUnknownNullCount. I'm not sure what we should do here, I think I need to know a lot more background. Can someone who has experience with null counts help?
Sorry I'm not sure if there is a well-documented rule of dealing with `kUnknownNullCount`, maybe @pitrou does? But in practice, I (and many other developers I suppose) just keep in mind that the null count can be unknown and is lazily computed. One can enforce such computation once the null count is absolutely needed (there are various APIs helping on this).
> 1. This sounds like we sometimes don't know the null count, I assume because it is expensive to compute. Would this be a good reason to actually force computation to happen (behavior A), or should we be conservative, and if we find kUnkownNullCount, then we assume that there could be nulls and don't let the cast happen and therefore error (behavior B). Behavior A seems much more correct to me.
Yeah your assumption is right. And I would go A (enforcing the null count computation) because IMO this is essential for what this PR is trying to do. Plus, null count is unknown by default, so B will make this change almost useless.
> 2. I don't think this function should have the responsibility of looking at the child arrays null counts. Should we add some higher level API to arrays like nullCountOrChildNullCount() that actually does this messy work??
IMO this is where you must make contact with child arrays, given that you are in the context of dealing with a `Struct` array - which is essentially a container of child arrays. And the original code already does so. Defining an API like `nullCountOrChildNullCount()` could be semantically hard/useless because: 1) the validity (null or not) of the whole struct and 2) the validity of each individual field (and you have to specify which field), mean very different things. So the null count of a particular field is a property of this particular field only and should be computed/checked individually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1943766445
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
+ return Status::TypeError("field '", target_field->name(),
Review Comment:
@mapleFU can you elaborate a little more? I don't think I understand neither why we should do this, or how. Does buffer[0] contain the null bitmap?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1951253170
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
Review Comment:
Thank you, good eye, I bet that is the problem. Another example of why we owe a beer to everyone who works on sanitizers/fuzzers/etc 🙇
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1945882938
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->GetNullCount() > 0) {
+ return Status::Invalid("field '", target_field->name(),
Review Comment:
Also `in_field->name()` and `in_field->type->ToString()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2656651990
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1949531790
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
+ const auto& out_field = out_type.field(out_field_index++);
+ const auto& in_field_type = in_field->type();
+ const auto& out_field_type = out_field->type();
+ if (in_field_index == kFillNullSentinel) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto nulls, MakeArrayOfNull(out_field_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
- const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
+ const auto& in_values = (in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
- ARROW_ASSIGN_OR_RAISE(Datum cast_values,
- Cast(values, target_type, options, ctx->exec_context()));
+ if (in_field->nullable() && !out_field->nullable() &&
Review Comment:
should we put an UNLIKELY compiler hint here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1949548098
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
+ const auto& out_field = out_type.field(out_field_index++);
+ const auto& in_field_type = in_field->type();
+ const auto& out_field_type = out_field->type();
+ if (in_field_index == kFillNullSentinel) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto nulls, MakeArrayOfNull(out_field_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
- const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
+ const auto& in_values = (in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
- ARROW_ASSIGN_OR_RAISE(Datum cast_values,
- Cast(values, target_type, options, ctx->exec_context()));
+ if (in_field->nullable() && !out_field->nullable() &&
Review Comment:
It's not really important, this is called only once per field (but if you want to, there's a `ARROW_PREDICT_FALSE` macro that you can use).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2303034536
<!--
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.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2650183017
Well, the Windows 2019 CI test shows a similar error, so I think it needs diagnosing and solving:
```
[ RUN ] Cast.StructToStructSubset
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\vector(1563) : Assertion failed: vector subscript out of range
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1889965297
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
+ return Status::TypeError("field '", target_field->name(),
Review Comment:
Besides, should we discard the `buffer[0]` if new data doesn't contains any nulls?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1890294211
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
I have never heard of kUnknownNullCount. I'm not sure what we should do here, I think I need to know a lot more background. Can someone who has experience with null counts help?
Some questions this brings up:
1. This sounds like we sometimes don't know the null count, I assume because it is expensive to compute. Would this be a good reason to actually force computation to happen (behavior A), or should we be conservative, and if we find kUnkownNullCount, then we assume that there could be nulls and don't let the cast happen and therefore error (behavior B). Behavior A seems much more correct to me.
2. I don't think this function should have the responsibility of looking at the child arrays null counts. Should we add some higher level API to arrays like nullCountOrChildNullCount() that actually does this messy work??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2357753994
Pushed a new version, hopefully that fixed the broken tests:
- To go from nullable to non-nullable type, need to use the unsafe cast option
- fix typo of int8 to int64 so the precision matches
- removed the [Go] tag from the commit message
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1889954570
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
I think `values->null_count` can likely to be `kUnknownNullCount`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1943766445
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
+ return Status::TypeError("field '", target_field->name(),
Review Comment:
@mapleFU can you elaborate a little more? I don't think I understand neither why we should do this, or how. Does buffer[0] contain the null bitmap, and if there are no nulls then we can get away without storing one??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946082497
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2974,18 +2975,20 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' of type int64 has nulls. Can't cast to field 'a' of non-nullable type int64"),
Cast(src_nullable, options2_non_nullable));
- std::vector<std::shared_ptr<Field>> fields_dest3_non_nullable = {
+ std::shared_ptr<Array> c_dest_no_nulls;
+ c_dest_no_nulls = ArrayFromJSON(int64(), "[9, 11, 44]");
Review Comment:
that seems like a good idea. Adding it now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2657082929
Thank you everyone! That was a lot of effort everyone did, I wouldn't have been able to do that without all the help. I'm excited for this to be released which will allow me to remove some hacky workarounds I have in my app code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2359850434
Ah, the Go part was removed from this PR.
I've removed "[Go]" from the PR title.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2648777160
I'm getting these failing UBSAN errors in CI. See below for the relevant logs. Is it because of the new `std::vector<std::shared_ptr<Array>> arrays_dest_ac = {arrays_dest[0], arrays_dest[2]}` code that I added? Is there a good way to silence/ignore this error?
<details>
<summary>Details</summary>
```
[ RUN ] Cast.StructToStructSubset
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1064:34: runtime error: addition of unsigned offset to 0x60700006f300 overflowed to 0x60700006f2e0
#0 0x7f811aca4bc8 in std::vector<std::shared_ptr<arrow::Field>, std::allocator<std::shared_ptr<arrow::Field> > >::operator[](unsigned long) const /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1064:34
#1 0x7f811ac9b27a in arrow::DataType::field(int) const /arrow/cpp/src/arrow/type.h:153:61
#2 0x7f8105e4f8f1 in arrow::compute::internal::(anonymous namespace)::CastStruct::Exec(arrow::compute::KernelContext*, arrow::compute::ExecSpan const&, arrow::compute::ExecResult*) /arrow/cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:403:38
#3 0x7f8105ab206e in arrow::compute::detail::(anonymous namespace)::ScalarExecutor::ExecuteNonSpans(arrow::compute::detail::ExecListener*) /arrow/cpp/src/arrow/compute/exec.cc:920:7
#4 0x7f8105aa99fa in arrow::compute::detail::(anonymous namespace)::ScalarExecutor::Execute(arrow::compute::ExecBatch const&, arrow::compute::detail::ExecListener*) /arrow/cpp/src/arrow/compute/exec.cc:810:14
#5 0x7f8105c471a6 in arrow::compute::detail::FunctionExecutorImpl::Execute(std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, long) /arrow/cpp/src/arrow/compute/function.cc:278:5
#6 0x7f8105c17bfe in arrow::compute::(anonymous namespace)::ExecuteInternal(arrow::compute::Function const&, std::vector<arrow::Datum, std::allocator<arrow::Datum> >, long, arrow::compute::FunctionOptions const*, arrow::compute::ExecContext*) /arrow/cpp/src/arrow/compute/function.cc:343:21
#7 0x7f8105c16d84 in arrow::compute::Function::Execute(std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, arrow::compute::FunctionOptions const*, arrow::compute::ExecContext*) const /arrow/cpp/src/arrow/compute/function.cc:350:10
#8 0x7f8105a5d13c in arrow::compute::internal::(anonymous namespace)::CastMetaFunction::ExecuteImpl(std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, arrow::compute::FunctionOptions const*, arrow::compute::ExecContext*) const /arrow/cpp/src/arrow/compute/cast.cc:124:23
#9 0x7f8105c21b5c in arrow::compute::MetaFunction::Execute(std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, arrow::compute::FunctionOptions const*, arrow::compute::ExecContext*) const /arrow/cpp/src/arrow/compute/function.cc:483:10
#10 0x7f8105a8cfc8 in arrow::compute::CallFunction(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, arrow::compute::FunctionOptions const*, arrow::compute::ExecContext*) /arrow/cpp/src/arrow/compute/exec.cc:1369:16
#11 0x55f68eeb23c9 in arrow::compute::CheckScalarNonRecursive(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, arrow::Datum const&, arrow::compute::FunctionOptions const*) /arrow/cpp/src/arrow/compute/kernels/test_util_internal.cc:80:3
#12 0x55f68eeb8480 in arrow::compute::CheckScalar(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<arrow::Datum, std::allocator<arrow::Datum> > const&, arrow::Datum, arrow::compute::FunctionOptions const*) /arrow/cpp/src/arrow/compute/kernels/test_util_internal.cc:109:3
#13 0x55f68eecc36c in arrow::compute::CheckScalarUnary(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, arrow::Datum, arrow::Datum, arrow::compute::FunctionOptions const*) /arrow/cpp/src/arrow/compute/kernels/test_util_internal.cc:255:3
#14 0x55f68ef4d8f7 in arrow::compute::CheckCast(std::shared_ptr<arrow::Array>, std::shared_ptr<arrow::Array>, arrow::compute::CastOptions) /arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc:111:3
#15 0x55f68f079ab8 in arrow::compute::CheckStructToStructSubset(std::vector<std::shared_ptr<arrow::DataType>, std::allocator<std::shared_ptr<arrow::DataType> > > const&) /arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc:3829:7
#16 0x55f68f06d09f in arrow::compute::Cast_StructToStructSubset_Test::TestBody() /arrow/cpp/src/arrow/compute/kernels/scalar_cast_test.cc:4003:36
#17 0x7f811af8260e in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/usr/local/lib/libarrow_testing.so.2000+0x108c60e) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#18 0x7f811af770c5 in testing::Test::Run() (/usr/local/lib/libarrow_testing.so.2000+0x10810c5) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#19 0x7f811af77244 in testing::TestInfo::Run() (/usr/local/lib/libarrow_testing.so.2000+0x1081244) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#20 0x7f811af777f8 in testing::TestSuite::Run() (/usr/local/lib/libarrow_testing.so.2000+0x10817f8) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#21 0x7f811af77efe in testing::internal::UnitTestImpl::RunAllTests() (/usr/local/lib/libarrow_testing.so.2000+0x1081efe) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#22 0x7f811af82bd6 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/usr/local/lib/libarrow_testing.so.2000+0x108cbd6) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#23 0x7f811af7730b in testing::UnitTest::Run() (/usr/local/lib/libarrow_testing.so.2000+0x108130b) (BuildId: 408b230591f17f240ad9d2b7d868274f1179009d)
#24 0x55f68edefb33 in main (/build/cpp/debug/arrow-compute-scalar-cast-test+0x2adb33) (BuildId: c176b331f0537175685c7888a83fc8e112006f6a)
#0 0x7f80f7d06d8f in
#26 0x7f80f7d06e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: cd410b710f0f094c6832edd95931006d883af48e)
#27 0x55f68edefb94 in _start (/build/cpp/debug/arrow-compute-scalar-cast-test+0x2adb94) (BuildId: c176b331f0537175685c7888a83fc8e112006f6a)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1064:34 in
/build/cpp/src/arrow/compute/kernels
Start 22: arrow-compute-scalar-cast-test
Test #22: arrow-compute-scalar-cast-test ...............***Failed 12.44 sec
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2651487788
I rebased on top of https://github.com/apache/arrow/pull/45500 so that hopefully CI will pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #43782:
URL: https://github.com/apache/arrow/pull/43782
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1943764878
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
OK, I adjusted to `values->GetNullCount() > 0`, I think we all the issues brought up here are resolved. re-open if I missed something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2303036852
:warning: GitHub issue #33592 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2352029525
They can land independently, no issues there. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1944374517
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2964,28 +2964,31 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest1_non_nullable = arrow::struct_(fields_dest1_non_nullable);
const auto options1_non_nullable = CastOptions::Safe(dest1_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' has nulls. Can't cast to non-nullable type int64"),
Cast(src_nullable, options1_non_nullable));
std::vector<std::shared_ptr<Field>> fields_dest2_non_nullable = {
std::make_shared<Field>("a", int64(), false),
std::make_shared<Field>("c", int64(), false)};
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
- const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
+ const auto options2_non_nullable = CastOptions::Unsafe(dest2_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' has nulls. Can't cast to non-nullable type int64"),
Cast(src_nullable, options2_non_nullable));
- std::vector<std::shared_ptr<Field>> fields_dest3_non_nullable = {
+ std::shared_ptr<Array> c_dest_no_nulls;
+ c_dest_no_nulls = ArrayFromJSON(int64(), "[9, 11, 44]");
+ std::vector<std::shared_ptr<Field>> fields_dest_no_nulls = {
std::make_shared<Field>("c", int64(), false)};
- const auto dest3_non_nullable = arrow::struct_(fields_dest3_non_nullable);
- const auto options3_non_nullable = CastOptions::Safe(dest3_non_nullable);
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
- Cast(src_nullable, options3_non_nullable));
+ ASSERT_OK_AND_ASSIGN(auto dest_no_nulls,
+ StructArray::Make({c_dest_no_nulls}, fields_dest_no_nulls));
+ const auto options3_non_nullable =
+ CastOptions::Unsafe(arrow::struct_(fields_dest_no_nulls));
Review Comment:
Similarly, why `Unsafe`?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2964,28 +2964,31 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest1_non_nullable = arrow::struct_(fields_dest1_non_nullable);
const auto options1_non_nullable = CastOptions::Safe(dest1_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' has nulls. Can't cast to non-nullable type int64"),
Cast(src_nullable, options1_non_nullable));
std::vector<std::shared_ptr<Field>> fields_dest2_non_nullable = {
std::make_shared<Field>("a", int64(), false),
std::make_shared<Field>("c", int64(), false)};
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
- const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
+ const auto options2_non_nullable = CastOptions::Unsafe(dest2_non_nullable);
Review Comment:
Why do we need to switch from `Safe` to `Unsafe` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1945883947
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->GetNullCount() > 0) {
Review Comment:
should we check `in_field->nullable()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946112138
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2974,18 +2975,20 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' of type int64 has nulls. Can't cast to field 'a' of non-nullable type int64"),
Cast(src_nullable, options2_non_nullable));
- std::vector<std::shared_ptr<Field>> fields_dest3_non_nullable = {
+ std::shared_ptr<Array> c_dest_no_nulls;
+ c_dest_no_nulls = ArrayFromJSON(int64(), "[9, 11, 44]");
Review Comment:
Added them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1891422950
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
You should just call `GetNullCount` so that the null count is computed if not already known.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2351813254
You probably need the same corresponding check on the Go side to verify that it's only allowed if there are no nulls.
Also, the Go implementation has been moved to the apache/arrow-go repository. So please file the PR there for the Go side. Sorry for the confusion, we just haven't removed the Go code from here yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2656658873
Revision: 4311938eb7f84d71a7fc04be591d2fdaa0e34293
Submitted crossbow builds: [ursacomputing/crossbow @ actions-f1ea7df869](https://github.com/ursacomputing/crossbow/branches/all?query=actions-f1ea7df869)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164590/job/37167089354)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164078/job/37167087474)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164436/job/37167088032)|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13309163852/job/37167085728)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/13309165019/job/37167090694)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13309165002/job/37167090719)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164579/job/37167089390)|
|test-cuda-cpp-ubuntu-20.04-cuda-11.2.2|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164433/job/37167088038)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164786/job/37167089758)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164748/job/37167089746)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164313/job/37167087437)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164797/job/37167089775)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13309163831/job/37167085650)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164630/job/37167089310)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164667/job/37167089388)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/13309163811/job/37167085676)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164097/job/37167087469)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164929/job/37167089782)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164294/job/37167087833)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/13309163732/job/37167085677)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/13309163914/job/37167086036)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164302/job/37167087855)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/13309164362/job/37167087466)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/13309165121/job/37167090710)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2638341303
@pitrou @mapleFU I applied your two suggestions, and I think this is ready for review again. Still one outstanding question about `buffer[0]`. CI is passing, except for one flaky test that failed during git checkout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946272769
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
+ const auto& out_field = out_type.field(out_field_index++);
+ const auto& in_field_type = in_field->type();
+ const auto& out_field_type = out_field->type();
+ if (in_field_index == kFillNullSentinel) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto nulls, MakeArrayOfNull(out_field_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
- const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
+ const auto& in_values = (in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
- ARROW_ASSIGN_OR_RAISE(Datum cast_values,
- Cast(values, target_type, options, ctx->exec_context()));
+ if (in_field->nullable() && !out_field->nullable() &&
+ in_values->GetNullCount() > 0) {
+ return Status::Invalid("field '", in_field->name(), "' of type ",
+ in_field_type->ToString(),
+ " has nulls. Can't cast to field '", out_field->name(),
+ "' of non-nullable type ", out_field_type->ToString());
Review Comment:
It's the field that's non-nullable
```suggestion
return Status::Invalid("field '", in_field->name(), "' of type ",
in_field_type->ToString(),
" has nulls. Can't cast to non-nullable field '",
out_field->name(), "' of type ",
out_field_type->ToString());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1945843491
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2964,28 +2964,31 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest1_non_nullable = arrow::struct_(fields_dest1_non_nullable);
const auto options1_non_nullable = CastOptions::Safe(dest1_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' has nulls. Can't cast to non-nullable type int64"),
Cast(src_nullable, options1_non_nullable));
std::vector<std::shared_ptr<Field>> fields_dest2_non_nullable = {
std::make_shared<Field>("a", int64(), false),
std::make_shared<Field>("c", int64(), false)};
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
- const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
+ const auto options2_non_nullable = CastOptions::Unsafe(dest2_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' has nulls. Can't cast to non-nullable type int64"),
Cast(src_nullable, options2_non_nullable));
- std::vector<std::shared_ptr<Field>> fields_dest3_non_nullable = {
+ std::shared_ptr<Array> c_dest_no_nulls;
+ c_dest_no_nulls = ArrayFromJSON(int64(), "[9, 11, 44]");
+ std::vector<std::shared_ptr<Field>> fields_dest_no_nulls = {
std::make_shared<Field>("c", int64(), false)};
- const auto dest3_non_nullable = arrow::struct_(fields_dest3_non_nullable);
- const auto options3_non_nullable = CastOptions::Safe(dest3_non_nullable);
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
- Cast(src_nullable, options3_non_nullable));
+ ASSERT_OK_AND_ASSIGN(auto dest_no_nulls,
+ StructArray::Make({c_dest_no_nulls}, fields_dest_no_nulls));
+ const auto options3_non_nullable =
+ CastOptions::Unsafe(arrow::struct_(fields_dest_no_nulls));
Review Comment:
Thanks, you are right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1889954570
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
I think `values->null_count` can likely to be `kUnknownNullCount`. `values->null_count > 0` means must has null, however, when values->null_count = kUnknownNullCount`, it would be wrongly casted to new field?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2351690898
Thanks for the help! I'm a random contributor, so I'm not sure about the exact workflow, but I think both getting the CI run approved and having a C++ owner approve it are needed.
Any more detailed thoughts on if the tests are adequate, if there's anywhere else in the code base that you think needs to change, my handling of the go implementation by just deleting the shortcircuit, or any other more detailed thoughts? Basically anything that would reduce the mental load on the code owner I think would increase the odds they approve it :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2351847627
Thanks @zeroshade , will do. Do we need both PRs to land at about the same time, or can I do that independently?
I will undo my changes to the go code here, leaving it untouched.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2353388063
Just pushed a new version:
- dropped the Go changes
- rebased on top of 3600db8c74f2d83850ad510aa7efbe10fddd9600, which is one commit behind [main](https://github.com/apache/arrow/commits/main/), because `main` is failing CI, but the commit I chose passed CI. I did this because I think the failing CI checks in this PR are not related to this PR
- I discovered `archery` and formatted the files with `archery lint --clang-format --fix`
We will see if this passes CI now...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2359849714
Can we move this to apache/arrow-go?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1889912258
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
Is it possible if `values->null_count` is `kUnknownNullCount`? In which case, we should probably use child array's `GetNullCount()` instead?
And if we are not sure about that, can we have a dedicated test for it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1891422950
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
Review Comment:
You should just call `GetNullCount` so that the null count is computed if not already known. Computation is reasonably cheap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946022935
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->GetNullCount() > 0) {
Review Comment:
Yeah this might cause the null bitmap being counted, I think check nullable would be faster than checking this if not neccessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2351679365
This PR looks pretty good to me, what additional help would you like getting it merged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2359080742
@zeroshade I think this is ready to review/merge, the failing CI runs look like flakes when trying to setup the environment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2351777409
Oops, didn't mean for this to still be marked WIP. Looks like a few failures that need to get fixed, but still a high-level review of the general approach would still be appreciated in the meantime.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2372555876
Those CI failures look unrelated? Not sure, [CI history looks pretty flaky](https://github.com/apache/arrow/commits/main/) to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1950464319
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
Review Comment:
This is probably the problem. You're indexing the fields with `in_field_index` *before* testing for `kFillNullSentinel`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1945843120
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2964,28 +2964,31 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest1_non_nullable = arrow::struct_(fields_dest1_non_nullable);
const auto options1_non_nullable = CastOptions::Safe(dest1_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' has nulls. Can't cast to non-nullable type int64"),
Cast(src_nullable, options1_non_nullable));
std::vector<std::shared_ptr<Field>> fields_dest2_non_nullable = {
std::make_shared<Field>("a", int64(), false),
std::make_shared<Field>("c", int64(), false)};
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
- const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
+ const auto options2_non_nullable = CastOptions::Unsafe(dest2_non_nullable);
Review Comment:
Thanks, you're right, I'm not sure why I had this change, maybe it wasn't passing in some earlier version of this changeset unless I had it. But it now works just fine with Safe, so reverting to that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1949503068
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
+ const auto& out_field = out_type.field(out_field_index++);
+ const auto& in_field_type = in_field->type();
+ const auto& out_field_type = out_field->type();
+ if (in_field_index == kFillNullSentinel) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto nulls, MakeArrayOfNull(out_field_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
- const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
+ const auto& in_values = (in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
- ARROW_ASSIGN_OR_RAISE(Datum cast_values,
- Cast(values, target_type, options, ctx->exec_context()));
+ if (in_field->nullable() && !out_field->nullable() &&
+ in_values->GetNullCount() > 0) {
+ return Status::Invalid("field '", in_field->name(), "' of type ",
+ in_field_type->ToString(),
+ " has nulls. Can't cast to field '", out_field->name(),
+ "' of non-nullable type ", out_field_type->ToString());
Review Comment:
good catch, thank you! Fixed now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946021105
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->GetNullCount() > 0) {
+ return Status::Invalid("field '", target_field->name(),
Review Comment:
Good idea, adding that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2642061587
whoops, I accidentally force-pushed with no changes, which closed the PR. Now it's back up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2656784843
The CI failures are unrelated (see https://github.com/apache/arrow/pull/45524).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592 [C++][Go]: support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2351691795
You can mark as ready for review? Or this is still wip?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2542689160
The current CI failures look to be caused by https://github.com/apache/arrow/issues/44985 (or similar), not by this PR.
I rebased on main, everything looks pretty much the same as before.
@zeroshade can you give this a review when you get the chance, or let me know how I can make it easier for you to do so? @rustyconover if you want to take a look again, maybe that will help. Thank you both!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1891442636
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
+ return Status::TypeError("field '", target_field->name(),
Review Comment:
Since this is data-dependent, this should be `Status::Invalid`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946020855
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->GetNullCount() > 0) {
Review Comment:
For correctness, I don't think this is needed (because the `values->GetNullCount() > 0` will take care of this), but for performance I could see it being nice to skip the `values->GetNullCount()` computation. I'll add the check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1946021721
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2974,18 +2975,20 @@ TEST(Cast, StructToDifferentNullabilityStruct) {
const auto dest2_non_nullable = arrow::struct_(fields_dest2_non_nullable);
const auto options2_non_nullable = CastOptions::Safe(dest2_non_nullable);
EXPECT_RAISES_WITH_MESSAGE_THAT(
- TypeError,
- ::testing::HasSubstr("cannot cast nullable field to non-nullable field"),
+ Invalid,
+ ::testing::HasSubstr(
+ "field 'a' of type int64 has nulls. Can't cast to field 'a' of non-nullable type int64"),
Cast(src_nullable, options2_non_nullable));
- std::vector<std::shared_ptr<Field>> fields_dest3_non_nullable = {
+ std::shared_ptr<Array> c_dest_no_nulls;
+ c_dest_no_nulls = ArrayFromJSON(int64(), "[9, 11, 44]");
Review Comment:
Can we add a test for sub slices? Like: `[null, 9, 11, 14]`, the `slice(0,2)` would cast failed, and `[11, 14]` would success?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews closed pull request #43782: GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values
URL: https://github.com/apache/arrow/pull/43782
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2657765600
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6a47e4d28cdc4592fe6a458dbe5efe3b17a090e5.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/37195291174) has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1949598718
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -403,17 +399,27 @@ struct CastStruct {
}
int out_field_index = 0;
- for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
- if (field_index == kFillNullSentinel) {
- ARROW_ASSIGN_OR_RAISE(auto nulls,
- MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
+ for (int in_field_index : fields_to_select) {
+ const auto& in_field = in_type.field(in_field_index);
+ const auto& out_field = out_type.field(out_field_index++);
+ const auto& in_field_type = in_field->type();
+ const auto& out_field_type = out_field->type();
+ if (in_field_index == kFillNullSentinel) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto nulls, MakeArrayOfNull(out_field_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
- const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
+ const auto& in_values = (in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
- ARROW_ASSIGN_OR_RAISE(Datum cast_values,
- Cast(values, target_type, options, ctx->exec_context()));
+ if (in_field->nullable() && !out_field->nullable() &&
Review Comment:
I'll keep it simple and leave it out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on code in PR #43782:
URL: https://github.com/apache/arrow/pull/43782#discussion_r1943763751
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -404,14 +400,20 @@ struct CastStruct {
int out_field_index = 0;
for (int field_index : fields_to_select) {
- const auto& target_type = out->type()->field(out_field_index++)->type();
+ const auto& target_field = out->type()->field(out_field_index++);
+ const auto& target_type = target_field->type();
if (field_index == kFillNullSentinel) {
ARROW_ASSIGN_OR_RAISE(auto nulls,
MakeArrayOfNull(target_type->GetSharedPtr(), batch.length));
out_array->child_data.push_back(nulls->data());
} else {
const auto& values = (in_array.child_data[field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+ if (!target_field->nullable() && values->null_count > 0) {
+ return Status::TypeError("field '", target_field->name(),
Review Comment:
Fixed this, thank you1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-33592: [C++] support casting nullable fields to non-nullable if there are no null values [arrow]
Posted by "NickCrews (via GitHub)" <gi...@apache.org>.
NickCrews commented on PR #43782:
URL: https://github.com/apache/arrow/pull/43782#issuecomment-2642194575
@mapleFU as I added the slicing tests, I also refactored the tests to make them much more consistent and concise. Take another look and make sure that your "LGTM" still holds after those changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org