ARROW-10924: [C++] Validate temporal data in ValidateArrayFull by JabariBooker · Pull Request #12014 · apache/arrow

JabariBooker · 2021-12-21T21:55:30Z

Validate that temporal data in arrays meet the restrictions of their data type specifications.

github-actions · 2021-12-21T21:55:51Z

https://issues.apache.org/jira/browse/ARROW-10924

github-actions · 2021-12-21T21:55:53Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

lidavidm

Thanks!

There are quite a few failing tests. We'll have to go through each of them individually to decide what to do about them.

You will want to lint and format the code.

cpp/src/arrow/array/validate.cc

lidavidm

Thanks for the updates.

You'll want to lint and format again (see the CI failure: https://github.com/apache/arrow/runs/4612596806?check_suite_focus=true)

Looking at test failures:

Random data generators need to be updated (also see these failures and these failures and there's more further down in the log)
bridge_test.cc needs updating
This constant also needs updating

cpp/src/arrow/array/validate.cc

edponce · 2021-12-29T20:39:35Z

@JabariBooker Thanks for working on this! I left some minor comments.
Also, we are missing validation for the following temporal types: Date32Type and TimestampType.

pitrou · 2021-12-30T17:44:59Z

@edponce There are no limits on Date32Type and TimestampType AFAICT.

edponce · 2022-01-06T15:21:22Z

@pitrou So there is a limit for Date64Type but not for Date32Type? I now understand that TimestampType does not have limits.

lidavidm · 2022-01-06T15:52:45Z

Date32Type's unit is days and so there's nothing to check, but Date64Type's unit is milliseconds, hence it doesn't have a limit per se, but it must be a multiple of 86400000 (=1 day in milliseconds).

…calar

lidavidm · 2022-01-11T13:24:46Z

FYI, that test failure in GroupBy.MinMaxTypes is here:

arrow/cpp/src/arrow/compute/kernels/hash_aggregate_test.cc

Line 1614 in d88e232

TEST(GroupBy, MinMaxTypes) {

The test uses one set of data to go through several data types, so you could just update the hardcoded data there too.

lidavidm

Thanks, LGTM. I left some nits (we can check DataType::id() instead of the string name). And thanks for going through all the function tests; we should perhaps consider removing Date64 from the list of types to check in order to just split off those tests separately given the restrictions on value. (EDIT: not in this PR of course.)

lidavidm · 2022-01-28T13:46:55Z

cpp/src/arrow/compute/kernels/hash_aggregate_test.cc

+    SCOPED_TRACE(ty->ToString());
+    auto in_schema = schema({field("argument0", ty), field("key", int64())});
+    auto table =
+        TableFromJSON(in_schema, (ty->name() == "date64") ? date64_table : default_table);


nit: can check ty->id() == Type::DATE64

lidavidm · 2022-01-28T13:47:05Z

cpp/src/arrow/compute/kernels/hash_aggregate_test.cc

-    [{"min": 3, "max": 5},       4],
-    [{"min": 1, "max": 4},       null]
-  ])"),
+            (ty->name() == "date64") ? date64_expected : default_expected),


nit: ditto here

lidavidm · 2022-01-28T13:47:23Z

cpp/src/arrow/compute/kernels/vector_hash_test.cc

-  // Sliced
-  CheckUnique(ArrayFromJSON(type, "[1, 2, null, 3, 2, null]")->Slice(1, 4),
-              ArrayFromJSON(type, "[2, null, 3]"));
+  if (type->name() == "date64") {


and here and so on (sorry)

lidavidm · 2022-01-28T13:51:18Z

cpp/src/arrow/testing/random.cc

+    }
+
+    case Type::type::TIME32: {
+      TimeUnit::type unit = std::dynamic_pointer_cast<Time32Type>(field.type())->unit();


nit: we have internal::checked_[pointer_]cast which will use dynamic or static cast depending on debug/release build

JabariBooker · 2022-01-28T16:02:16Z

@lidavidm @edponce @pitrou There are still failing integration tests (all are also failing due to invalid values for date64). The data involved in these tests seem to be generated by some mechanism I'm unaware of.

cpp/src/arrow/array/array_test.cc

edponce · 2022-01-28T19:31:19Z

cpp/src/arrow/testing/random.cc

  options.GenerateData(buffers[1]->mutable_data(), size);
+  if (std::is_same<ArrowType, Date64Type>::value) {
+    GenerateFullDayMillisNoNan(buffers[1]->mutable_data(), size);
+  }


This should be an if-else. buffers[1]->mutable_data() is modified twice if Date64Type.

I think a more suitable place for this type-specific generation of numeric arrays, is in GenerateTypeDataNoNan() from GenerateOptions. Make GenerateFullDayMillisNoNan as a private member to GenerateOptions and perform the if-else check in GenerateTypedDataNoNan().

This is intentional. Since we need to generate multiples of 86400000 for Date64Type, random numbers are generated in GenerateData and ultimately multiplied by 86400000 in GenerateFullDayMillisNoNan. The behavior of GenerateFullDayMillisNoNan is separated from GenerateOptions since this class doesn't know the arrow type, only the underlying C type. However, behavior of GenerateFullDayMillisNoNan could be moved into the above if statement if needed.

cpp/src/arrow/testing/random.cc

pitrou · 2022-01-31T10:40:03Z

@JabariBooker Integration data is generated in Python by Archery here: https://github.com/apache/arrow/blob/master/dev/archery/archery/integration/datagen.py#L196

You can find more info about the JSON integration format here, and how to run the integration tests locally, here: https://arrow.apache.org/docs/format/Integration.html

lidavidm

Thanks, just a minor nit on the datagen (which we probably won't run into, but…)

lidavidm · 2022-02-01T13:12:08Z

dev/archery/archery/integration/datagen.py

+            return super().generate_range(size, lower, upper, name)
+
+        full_day_millis = 1000 * 60 * 60 * 24
+        lower //= full_day_millis


Python appears to round towards -Inf so a lower negative bound that is not exactly even will get rounded to an out-of-bound value

lidavidm

Thanks, I think we should be good here. This turned out to be quite a bit more involved than it looked at first.

@pitrou @edponce any other comments?

dev/archery/archery/integration/tester_cpp.py

ursabot · 2022-02-02T14:51:08Z

Benchmark runs are scheduled for baseline = 9b53235 and contender = c715beb. c715beb is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

JabariBooker added 2 commits December 21, 2021 14:40

Adding validation for Date64Type, Time32Type, and Time64Type

359b29e

Merge branch 'master' of https://github.com/apache/arrow

fd2e08d

github-actions bot added the Component: C++ label Dec 21, 2021

Corrected type usage for Time64Type check

b13946b

lidavidm reviewed Dec 22, 2021

View reviewed changes

cpp/src/arrow/array/validate.cc Outdated Show resolved Hide resolved

cpp/src/arrow/array/validate.cc Outdated Show resolved Hide resolved

cpp/src/arrow/array/validate.cc Outdated Show resolved Hide resolved

JabariBooker added 3 commits December 22, 2021 16:31

Resolving formatting issues and adding constexpr's

bd1209e

Using kCamelCase for constants

aeedda5

Renaming time constants

9400fbb

lidavidm reviewed Dec 23, 2021

View reviewed changes

edponce suggested changes Dec 29, 2021

View reviewed changes

Merge branch 'master' into ARROW-10924

ec369c4

JabariBooker added 9 commits January 7, 2022 00:43

Merge branch 'master' into ARROW-10924

68f62a5

Moving constexpr's and making small grammatical edits

f68c2a8

More grammar corrections and updating array test constant for Date64S…

827b6a4

…calar

Added random generation handling for temporal types

4ace496

Merge branch 'master' into ARROW-10924

b7b9d16

Formatting code properly

36c3dfc

Changing test constants to reflect new restrictions

6d7189b

Added new test buffers for date64

d8be683

Fixed random number generation and constants for testing date64

997c7a1

JabariBooker added 2 commits January 27, 2022 20:47

Changes to the last unit tests for date64 restrictions

e409c73

Missing changes to constant from last commit

27411f2

lidavidm approved these changes Jan 28, 2022

View reviewed changes

lidavidm mentioned this pull request Jan 28, 2022

ARROW-14097: [C++] subtract(time, duration) -> time kernel #12139

Closed

edponce reviewed Jan 28, 2022

View reviewed changes

cpp/src/arrow/array/array_test.cc Show resolved Hide resolved

edponce suggested changes Jan 28, 2022

View reviewed changes

JabariBooker added 2 commits January 31, 2022 15:48

Updated data generation for integration tests; other minor changes

75febd1

Added flag for date64 validation in integration tests, set to false

e1ba73a

lidavidm reviewed Feb 1, 2022

View reviewed changes

Added integration test flag for TIME32/64 types

cc7192f

lidavidm reviewed Feb 1, 2022

View reviewed changes

dev/archery/archery/integration/tester_cpp.py Show resolved Hide resolved

edponce approved these changes Feb 2, 2022

View reviewed changes

lidavidm closed this in c715beb Feb 2, 2022

asfimport mentioned this pull request Feb 3, 2022

[C++] Validate temporal data in ValidateArrayFull #26852

Closed

Jefffrey mentioned this pull request Jan 7, 2024

Confusion over Date64 array values apache/arrow-rs#5288

Closed

Conversation

JabariBooker commented Dec 21, 2021

Uh oh!

github-actions bot commented Dec 21, 2021

Uh oh!

github-actions bot commented Dec 21, 2021

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lidavidm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edponce commented Dec 29, 2021

Uh oh!

pitrou commented Dec 30, 2021

Uh oh!

edponce commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented Jan 6, 2022

Uh oh!

lidavidm commented Jan 11, 2022

Uh oh!

lidavidm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidavidm Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

lidavidm Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

lidavidm Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

lidavidm Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

JabariBooker commented Jan 28, 2022

Uh oh!

Uh oh!

edponce Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

edponce Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

JabariBooker Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Jan 31, 2022

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ursabot commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

lidavidm left a comment •

edited

Loading

edponce commented Jan 6, 2022 •

edited

Loading

lidavidm left a comment •

edited

Loading

JabariBooker Jan 28, 2022 •

edited

Loading

ursabot commented Feb 2, 2022 •

edited

Loading