edponce commented on a change in pull request #12014:
URL: https://github.com/apache/arrow/pull/12014#discussion_r776479002
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -166,6 +166,80 @@ struct ValidateArrayImpl {
return Status::OK();
}
+ Status Visit(const Date64Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Date64Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDay = 1000 * 60 * 60 * 24;
+ return VisitArrayDataInline<Date64Type>(
+ data,
+ [&](c_type date) {
+ if (date % kFullDay != 0) {
+ return Status::Invalid(type, date,
+ "ms does not represent a whole number of
days");
Review comment:
Is a whitespace missing between `type` and `date`?
Also, it should be _"does not represents"_ ('s' at the end).
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -166,6 +166,80 @@ struct ValidateArrayImpl {
return Status::OK();
}
+ Status Visit(const Date64Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Date64Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDay = 1000 * 60 * 60 * 24;
Review comment:
`kFullDay` --> `kFullDayMillis` to be consistent with the other cases.
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -166,6 +166,80 @@ struct ValidateArrayImpl {
return Status::OK();
}
+ Status Visit(const Date64Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Date64Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDay = 1000 * 60 * 60 * 24;
Review comment:
Also, I suggest to move these constexpr statements inside the lambdas.
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -166,6 +166,80 @@ struct ValidateArrayImpl {
return Status::OK();
}
+ Status Visit(const Date64Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Date64Type::c_type;
Review comment:
Nit: Move `using` statement inside `if (full_validation) { .. }` block.
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -166,6 +166,80 @@ struct ValidateArrayImpl {
return Status::OK();
}
+ Status Visit(const Date64Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Date64Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDay = 1000 * 60 * 60 * 24;
+ return VisitArrayDataInline<Date64Type>(
+ data,
+ [&](c_type date) {
+ if (date % kFullDay != 0) {
+ return Status::Invalid(type, date,
+ "ms does not represent a whole number of
days");
+ }
+ return Status::OK();
+ },
+ []() { return Status::OK(); });
+ }
+ return Status::OK();
+ }
+
+ Status Visit(const Time32Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Time32Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDaySeconds = 60 * 60 * 24;
+ constexpr c_type kFullDayMillis = kFullDaySeconds * 1000;
+ return VisitArrayDataInline<Time32Type>(
+ data,
+ [&](c_type time) {
+ if (type.unit() == TimeUnit::SECOND && (time < 0 || time >=
kFullDaySeconds)) {
+ return Status::Invalid(type, " ", time,
+ "s does not fit within the acceptable
range of ",
Review comment:
_"does not fit within"_ --> _"is not within"_ I think this message
represents better the invalid value.
Same applies for the other cases below.
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -166,6 +166,80 @@ struct ValidateArrayImpl {
return Status::OK();
}
+ Status Visit(const Date64Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Date64Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDay = 1000 * 60 * 60 * 24;
+ return VisitArrayDataInline<Date64Type>(
+ data,
+ [&](c_type date) {
+ if (date % kFullDay != 0) {
+ return Status::Invalid(type, date,
+ "ms does not represent a whole number of
days");
+ }
+ return Status::OK();
+ },
+ []() { return Status::OK(); });
+ }
+ return Status::OK();
+ }
+
+ Status Visit(const Time32Type& type) {
+ RETURN_NOT_OK(ValidateFixedWidthBuffers());
+
+ using c_type = typename Time32Type::c_type;
+ if (full_validation) {
+ constexpr c_type kFullDaySeconds = 60 * 60 * 24;
+ constexpr c_type kFullDayMillis = kFullDaySeconds * 1000;
+ return VisitArrayDataInline<Time32Type>(
Review comment:
Move these `constexpr` variables inside the lambda before their
respective 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
