Re: [PR] GH-44054: [C++] Implement PostgreSQL dedicated date extracting functions [arrow]

["ZhangHuiGui (via GitHub)" <gi...@apache.org>]

ZhangHuiGui commented on code in PR #44057:
URL: https://github.com/apache/arrow/pull/44057#discussion_r1772582207


##########
cpp/src/arrow/vendored/datetime/date.h:
##########
@@ -745,6 +750,65 @@ template<class CharT, class Traits>
 std::basic_ostream<CharT, Traits>&
 operator<<(std::basic_ostream<CharT, Traits>& os, const year_month_day& ymd);
 
+// class year_month_day_pg
+
+class year_month_day_pg {

Review Comment:
   Is there a way to simplify this class? The difference from the original `year_month_day` is mainly in `from_days` and `to_days`. Year_month_day can be used as a class template. We can specialize different types of implementations. PostgreSQL's `to_days/from_days` is one of the specializations, and other functions can use the default ones.
   
   Otherwise, there will be a lot of code redundancy when we need to be compatible with the temporal functions of other databases/data management systems in the future.



##########
cpp/src/arrow/compute/api_scalar.cc:
##########
@@ -864,6 +864,9 @@ SCALAR_EAGER_UNARY(Millisecond, "millisecond")
 SCALAR_EAGER_UNARY(Minute, "minute")
 SCALAR_EAGER_UNARY(Month, "month")
 SCALAR_EAGER_UNARY(Nanosecond, "nanosecond")
+SCALAR_EAGER_UNARY(PGDay, "pg_day")
+SCALAR_EAGER_UNARY(PGMonth, "pg_month")
+SCALAR_EAGER_UNARY(PGYear, "pg_year")

Review Comment:
   `pg_week` can also be supported to provide the same capabilities as `ISOWeek` and `USWeek`.



##########
cpp/src/arrow/compute/api_scalar.h:
##########
@@ -1332,6 +1332,40 @@ Result<Datum> ISOYear(const Datum& values, ExecContext* ctx = NULLPTR);
 ARROW_EXPORT
 Result<Datum> USYear(const Datum& values, ExecContext* ctx = NULLPTR);
 
+/// \brief Year returns PostgreSQL year for each element of `values`

Review Comment:
   Here can be a more detailed description of `PGYear` and other types of Year, where are the main differences.



##########
cpp/src/arrow/vendored/datetime/date.h:
##########
@@ -89,6 +89,11 @@ namespace arrow_vendored
 namespace date
 {
 
+// 10957 = julian days of '2000-01-01' - julian days of '1970-01-01'
+// This is used to align the number of days based on the PG epoch to

Review Comment:
   It is best to use the full name of `PostgreSQL` here rather than `PG`.



-- 
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