wgtmac commented on code in PR #185: URL: https://github.com/apache/iceberg-cpp/pull/185#discussion_r2299673313
########## src/iceberg/util/literal_format.cc: ########## @@ -0,0 +1,76 @@ +/* + * 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. + */ + +#include "iceberg/util/literal_format.h" + +#include <chrono> +#include <cstring> +#include <iomanip> + +namespace iceberg { + +std::string FormatDate(int32_t days_since_epoch) { + // Convert days since Unix epoch to date + auto time_point = + std::chrono::system_clock::time_point{} + std::chrono::days{days_since_epoch}; + auto date = std::chrono::floor<std::chrono::days>(time_point); + auto ymd = std::chrono::year_month_day{date}; + + std::ostringstream oss; + oss << static_cast<int>(ymd.year()) << "-" << std::setfill('0') << std::setw(2) + << static_cast<unsigned>(ymd.month()) << "-" << std::setfill('0') << std::setw(2) + << static_cast<unsigned>(ymd.day()); + return oss.str(); +} + +std::string FormatTime(int64_t microseconds_since_midnight) { + auto hours = microseconds_since_midnight / (1000000LL * 3600); + auto minutes = (microseconds_since_midnight % (1000000LL * 3600)) / (1000000LL * 60); + auto seconds = (microseconds_since_midnight % (1000000LL * 60)) / 1000000LL; + auto micros = microseconds_since_midnight % 1000000LL; + + std::ostringstream oss; + oss << std::setfill('0') << std::setw(2) << hours << ":" << std::setfill('0') + << std::setw(2) << minutes << ":" << std::setfill('0') << std::setw(2) << seconds + << "." << std::setfill('0') << std::setw(6) << micros; + return oss.str(); +} + +std::string FormatTimestamp(int64_t microseconds_since_epoch) { Review Comment: Please note that `timestamp` and `timestamptz` are different. `timestamp` should print the text in local timezone while `timestamptz` should use UTC. ########## src/iceberg/util/endian.h: ########## @@ -0,0 +1,93 @@ +/* + * 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. + */ + +#pragma once + +#include <algorithm> +#include <bit> +#include <cstring> +#include <span> +#include <vector> + +#include "iceberg/result.h" + +/// \file iceberg/util/endian.h +/// \brief Endianness conversion utilities + +namespace iceberg::util { Review Comment: This isn't resolved, right? ########## src/iceberg/util/literal_format.cc: ########## @@ -0,0 +1,76 @@ +/* + * 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. + */ + +#include "iceberg/util/literal_format.h" Review Comment: It would be better if we can split it (i.e. adding `date_time_util.h/.cc` and do the suggested refactoring from transfrom_function.cc) from this PR and check in that first. ########## src/iceberg/util/literal_format.cc: ########## @@ -0,0 +1,76 @@ +/* + * 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. + */ + +#include "iceberg/util/literal_format.h" Review Comment: ```suggestion #include "iceberg/util/date_time_util.h" ``` I think we should eventually add all functions from https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/util/DateTimeUtil.java. So let's rename it first. We need also move all time transformation to this utility: https://github.com/apache/iceberg-cpp/blob/main/src/iceberg/transform_function.cc#L204-L405 ########## src/iceberg/util/literal_format.cc: ########## @@ -0,0 +1,76 @@ +/* + * 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. + */ + +#include "iceberg/util/literal_format.h" + +#include <chrono> +#include <cstring> +#include <iomanip> + +namespace iceberg { + +std::string FormatDate(int32_t days_since_epoch) { Review Comment: Could you please leverage std::formatter https://en.cppreference.com/w/cpp/chrono/hh_mm_ss/formatter.html instead of manual conversions? There are a bunch of specialization for `std::formatter<std::chrono::xyz>`. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
