You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/30 09:01:34 UTC
[GitHub] [arrow] rtpsw opened a new pull request, #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
rtpsw opened a new pull request, #13037:
URL: https://github.com/apache/arrow/pull/13037
See https://issues.apache.org/jira/browse/ARROW-16425
--
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
[GitHub] [arrow] rtpsw commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1113953353
cc @icexelloss
--
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
[GitHub] [arrow] lidavidm closed pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
URL: https://github.com/apache/arrow/pull/13037
--
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
[GitHub] [arrow] lidavidm commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1122506399
Yeah, sorry - just realized the declaration syntax requires you to repeat the `&` in order to actually make it a reference
--
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
[GitHub] [arrow] rtpsw commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1114281826
The build errors don't seem to be related to the change. Please advise how to proceed.
--
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
[GitHub] [arrow] rtpsw commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1122434452
@lidavidm, do you mean [the two suggestions here](https://github.com/apache/arrow/pull/13037#pullrequestreview-962207283)? I see they are partly addressed [here](https://github.com/apache/arrow/pull/13037/files#diff-52cb8693a09d5122506af72ba32737da4bd426713cc41dff2de99c4b1a7f078cR523) and here; in both cases, the `&` in the second variable is still missing. I'll fix shortly.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1125375816
Sorry for the delay here, and thanks for the updates
--
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
[GitHub] [arrow] westonpace commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
westonpace commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1121780723
Sorry, didn't mean to re-request review :grimacing:
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13037:
URL: https://github.com/apache/arrow/pull/13037#discussion_r865076491
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -503,23 +503,24 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
TEST(TestCompareTimestamps, ScalarArray) {
- const char* scalar_json = R"("1970-01-02")";
- const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
+ std::string scalar_json = R"("1970-01-02")";
+ std::string array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
struct ArrayCase {
Datum side1, side2, expected;
};
auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
std::shared_ptr<DataType> array_type, CompareOperator op,
- const char* expected_json, const char* flip_expected_json) {
+ const std::string& expected_json,
+ const std::string& flip_expected_json) {
auto scalar_side = ScalarFromJSON(scalar_type, scalar_json);
auto array_side = ArrayFromJSON(array_type, array_json);
auto expected = ArrayFromJSON(boolean(), expected_json);
auto flip_expected = ArrayFromJSON(boolean(), flip_expected_json);
- for (auto array_case :
+ for (const auto& array_case :
std::vector<ArrayCase>{{scalar_side, array_side, expected},
{array_side, scalar_side, flip_expected}}) {
- auto lhs = array_case.side1, rhs = array_case.side2;
+ const auto &lhs = array_case.side1, rhs = array_case.side2;
Review Comment:
```suggestion
const auto &lhs = array_case.side1, &rhs = array_case.side2;
```
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -535,15 +536,15 @@ TEST(TestCompareTimestamps, ScalarArray) {
}
};
- for (auto unit : TimeUnit::values()) {
- for (auto types :
+ for (const auto& unit : TimeUnit::values()) {
+ for (const auto& types :
std::vector<std::pair<std::shared_ptr<DataType>, std::shared_ptr<DataType>>>{
{timestamp(unit), timestamp(unit)},
{timestamp(unit), timestamp(unit, "utc")},
{timestamp(unit, "utc"), timestamp(unit)},
{timestamp(unit, "utc"), timestamp(unit, "utc")},
}) {
- auto t0 = types.first, t1 = types.second;
+ const auto &t0 = types.first, t1 = types.second;
Review Comment:
```suggestion
const auto &t0 = types.first, &t1 = types.second;
```
--
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
[GitHub] [arrow] rtpsw commented on a diff in pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13037:
URL: https://github.com/apache/arrow/pull/13037#discussion_r863550292
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
+
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json) {
+ auto lhs = ScalarFromJSON(scalar_type, scalar_json);
+ auto rhs = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*expected, *result.make_array(), /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ };
+
+ for (auto unit : {
+ TimeUnit::SECOND,
+ TimeUnit::MILLI,
+ TimeUnit::MICRO,
+ TimeUnit::NANO,
+ }) {
+ for (auto types :
+ std::vector<std::pair<std::shared_ptr<DataType>, std::shared_ptr<DataType>>>{
+ {timestamp(unit), timestamp(unit)},
+ {timestamp(unit), timestamp(unit, "utc")},
+ {timestamp(unit, "utc"), timestamp(unit)},
+ {timestamp(unit, "utc"), timestamp(unit, "utc")},
+ }) {
+ auto t0 = types.first, t1 = types.second;
+ CheckArrayCase(t0, t1, CompareOperator::EQUAL, "[true, false, false]");
Review Comment:
Included in recent commit.
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
Review Comment:
Included in recent commit.
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13037:
URL: https://github.com/apache/arrow/pull/13037#discussion_r864765944
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,64 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
+
+ struct ArrayCase {
+ Datum side1, side2, expected;
+ };
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json, const char* flip_expected_json) {
Review Comment:
`const std::string&`?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,64 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
Review Comment:
Can we use std::string?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,64 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
+
+ struct ArrayCase {
+ Datum side1, side2, expected;
+ };
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json, const char* flip_expected_json) {
+ auto scalar_side = ScalarFromJSON(scalar_type, scalar_json);
+ auto array_side = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ auto flip_expected = ArrayFromJSON(boolean(), flip_expected_json);
+ for (auto array_case :
+ std::vector<ArrayCase>{{scalar_side, array_side, expected},
+ {array_side, scalar_side, flip_expected}}) {
+ auto lhs = array_case.side1, rhs = array_case.side2;
Review Comment:
`const auto&`?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,64 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
+
+ struct ArrayCase {
+ Datum side1, side2, expected;
+ };
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json, const char* flip_expected_json) {
+ auto scalar_side = ScalarFromJSON(scalar_type, scalar_json);
+ auto array_side = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ auto flip_expected = ArrayFromJSON(boolean(), flip_expected_json);
+ for (auto array_case :
+ std::vector<ArrayCase>{{scalar_side, array_side, expected},
+ {array_side, scalar_side, flip_expected}}) {
+ auto lhs = array_case.side1, rhs = array_case.side2;
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*array_case.expected.make_array(), *result.make_array(),
+ /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ }
+ };
+
+ for (auto unit : TimeUnit::values()) {
+ for (auto types :
Review Comment:
`const auto&`?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,64 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
+
+ struct ArrayCase {
+ Datum side1, side2, expected;
+ };
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json, const char* flip_expected_json) {
+ auto scalar_side = ScalarFromJSON(scalar_type, scalar_json);
+ auto array_side = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ auto flip_expected = ArrayFromJSON(boolean(), flip_expected_json);
+ for (auto array_case :
Review Comment:
`const auto&`?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,64 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01",null,"1900-02-28"])";
+
+ struct ArrayCase {
+ Datum side1, side2, expected;
+ };
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json, const char* flip_expected_json) {
+ auto scalar_side = ScalarFromJSON(scalar_type, scalar_json);
+ auto array_side = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ auto flip_expected = ArrayFromJSON(boolean(), flip_expected_json);
+ for (auto array_case :
+ std::vector<ArrayCase>{{scalar_side, array_side, expected},
+ {array_side, scalar_side, flip_expected}}) {
+ auto lhs = array_case.side1, rhs = array_case.side2;
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*array_case.expected.make_array(), *result.make_array(),
+ /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ }
+ };
+
+ for (auto unit : TimeUnit::values()) {
+ for (auto types :
+ std::vector<std::pair<std::shared_ptr<DataType>, std::shared_ptr<DataType>>>{
+ {timestamp(unit), timestamp(unit)},
+ {timestamp(unit), timestamp(unit, "utc")},
+ {timestamp(unit, "utc"), timestamp(unit)},
+ {timestamp(unit, "utc"), timestamp(unit, "utc")},
+ }) {
+ auto t0 = types.first, t1 = types.second;
Review Comment:
`const auto&`?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1113953253
https://issues.apache.org/jira/browse/ARROW-16425
--
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
[GitHub] [arrow] rok commented on a diff in pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rok commented on code in PR #13037:
URL: https://github.com/apache/arrow/pull/13037#discussion_r862551524
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
+
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json) {
+ auto lhs = ScalarFromJSON(scalar_type, scalar_json);
+ auto rhs = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*expected, *result.make_array(), /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ };
+
+ for (auto unit : {
+ TimeUnit::SECOND,
+ TimeUnit::MILLI,
+ TimeUnit::MICRO,
+ TimeUnit::NANO,
+ }) {
+ for (auto types :
+ std::vector<std::pair<std::shared_ptr<DataType>, std::shared_ptr<DataType>>>{
+ {timestamp(unit), timestamp(unit)},
+ {timestamp(unit), timestamp(unit, "utc")},
+ {timestamp(unit, "utc"), timestamp(unit)},
+ {timestamp(unit, "utc"), timestamp(unit, "utc")},
+ }) {
+ auto t0 = types.first, t1 = types.second;
+ CheckArrayCase(t0, t1, CompareOperator::EQUAL, "[true, false, false]");
Review Comment:
Could `CheckScalarBinaryCommutative` or `CheckScalarBinary` work here?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
+
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json) {
+ auto lhs = ScalarFromJSON(scalar_type, scalar_json);
+ auto rhs = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*expected, *result.make_array(), /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ };
+
+ for (auto unit : {
+ TimeUnit::SECOND,
+ TimeUnit::MILLI,
+ TimeUnit::MICRO,
+ TimeUnit::NANO,
+ }) {
Review Comment:
```suggestion
for (auto unit : TimeUnit::values()) {
```
--
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
[GitHub] [arrow] ursabot commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1126806774
Benchmark runs are scheduled for baseline = 06531131b292830db29054e4128819bf25ace794 and contender = ebfbb08d8e62fc788caab2c3a9958f34cd17a26a. ebfbb08d8e62fc788caab2c3a9958f34cd17a26a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/23441ec320694c3a8396ae1b80c7e4c8...0fff84fc32b142c093cc237864a8eceb/)
[Failed :arrow_down:0.08% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/4d069b00432846c0af392d4145a97f7b...39e1a6a3a1274ee5b78fced4f9dcd623/)
[Finished :arrow_down:0.36% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a6482da70eb6452997dbfe58e8f45492...fabe8f1e57db4973b01e04bd2c1d58c8/)
[Finished :arrow_down:0.16% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/6bfe42167e584dbe821e4caad95d30d0...b745eeb64966426992d3ba577760bd2f/)
Buildkite builds:
[Finished] [`ebfbb08d` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/753)
[Failed] [`ebfbb08d` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/750)
[Finished] [`ebfbb08d` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/740)
[Finished] [`ebfbb08d` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/755)
[Finished] [`06531131` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/752)
[Finished] [`06531131` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/749)
[Finished] [`06531131` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/739)
[Finished] [`06531131` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/754)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] rtpsw commented on a diff in pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rtpsw commented on code in PR #13037:
URL: https://github.com/apache/arrow/pull/13037#discussion_r863549760
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
+
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json) {
+ auto lhs = ScalarFromJSON(scalar_type, scalar_json);
+ auto rhs = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*expected, *result.make_array(), /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ };
+
+ for (auto unit : {
+ TimeUnit::SECOND,
+ TimeUnit::MILLI,
+ TimeUnit::MICRO,
+ TimeUnit::NANO,
+ }) {
Review Comment:
Included in recent commit.
--
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
[GitHub] [arrow] lidavidm commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1122312377
In any case it seems the last two suggestions are unaddressed?
--
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
[GitHub] [arrow] lidavidm commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1117562953
Thanks, looks like the formatter is pointing out that the declarations need to be separated:
```diff
--- /arrow/cpp/src/arrow/compute/kernels/scalar_compare_test.cc
+++ /arrow/cpp/src/arrow/compute/kernels/scalar_compare_test.cc (after clang format)
@@ -520,7 +520,7 @@
for (const auto& array_case :
std::vector<ArrayCase>{{scalar_side, array_side, expected},
{array_side, scalar_side, flip_expected}}) {
- const auto& lhs = array_case.side1, rhs = array_case.side2;
+ const auto &lhs = array_case.side1, rhs = array_case.side2;
if (scalar_type->Equals(array_type)) {
ASSERT_OK_AND_ASSIGN(Datum result,
CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
@@ -544,7 +544,7 @@
{timestamp(unit, "utc"), timestamp(unit)},
{timestamp(unit, "utc"), timestamp(unit, "utc")},
}) {
- const auto& t0 = types.first, t1 = types.second;
+ const auto &t0 = types.first, t1 = types.second;
CheckArrayCase(t0, t1, CompareOperator::EQUAL, "[true, false, null, false]",
"[true, false, null, false]");
CheckArrayCase(t0, t1, CompareOperator::NOT_EQUAL, "[false, true, null, true]",
/arrow/cpp/src/arrow/compute/kernels/scalar_compare_test.cc had clang-format style issues
```
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13037:
URL: https://github.com/apache/arrow/pull/13037#discussion_r862789676
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
Review Comment:
Can we add a `null` element here too?
##########
cpp/src/arrow/compute/kernels/scalar_compare_test.cc:
##########
@@ -502,6 +502,53 @@ TEST(TestCompareTimestamps, DifferentParameters) {
}
}
+TEST(TestCompareTimestamps, ScalarArray) {
+ const char* scalar_json = R"("1970-01-02")";
+ const char* array_json = R"(["1970-01-02","2000-02-01","1900-02-28"])";
+
+ auto CheckArrayCase = [&](std::shared_ptr<DataType> scalar_type,
+ std::shared_ptr<DataType> array_type, CompareOperator op,
+ const char* expected_json) {
+ auto lhs = ScalarFromJSON(scalar_type, scalar_json);
+ auto rhs = ArrayFromJSON(array_type, array_json);
+ auto expected = ArrayFromJSON(boolean(), expected_json);
+ if (scalar_type->Equals(array_type)) {
+ ASSERT_OK_AND_ASSIGN(Datum result,
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ AssertArraysEqual(*expected, *result.make_array(), /*verbose=*/true);
+ } else {
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid,
+ ::testing::HasSubstr(
+ "Cannot compare timestamp with timezone to timestamp without timezone"),
+ CallFunction(CompareOperatorToFunctionName(op), {lhs, rhs}));
+ }
+ };
+
+ for (auto unit : {
+ TimeUnit::SECOND,
+ TimeUnit::MILLI,
+ TimeUnit::MICRO,
+ TimeUnit::NANO,
+ }) {
+ for (auto types :
+ std::vector<std::pair<std::shared_ptr<DataType>, std::shared_ptr<DataType>>>{
+ {timestamp(unit), timestamp(unit)},
+ {timestamp(unit), timestamp(unit, "utc")},
+ {timestamp(unit, "utc"), timestamp(unit)},
+ {timestamp(unit, "utc"), timestamp(unit, "utc")},
+ }) {
+ auto t0 = types.first, t1 = types.second;
+ CheckArrayCase(t0, t1, CompareOperator::EQUAL, "[true, false, false]");
Review Comment:
At the very least we should test that the comparison works when the scalar is second. And then it would likely be easier to write it out instead of trying to factor out a helper function.
--
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
[GitHub] [arrow] ursabot commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1126806790
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/a6482da70eb6452997dbfe58e8f45492...fabe8f1e57db4973b01e04bd2c1d58c8/)
--
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
[GitHub] [arrow] rtpsw commented on pull request #13037: ARROW-16425: [C++] Add compute kernel test for scalar array timestamp comparison
Posted by GitBox <gi...@apache.org>.
rtpsw commented on PR #13037:
URL: https://github.com/apache/arrow/pull/13037#issuecomment-1122538382
The Travis CI error is unrelated to the change.
--
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