Copilot commented on code in PR #701: URL: https://github.com/apache/iceberg-cpp/pull/701#discussion_r3636012117
########## src/iceberg/expression/sanitize_expression.cc: ########## @@ -0,0 +1,353 @@ +/* + * 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/expression/sanitize_expression.h" + +#include <chrono> +#include <cmath> +#include <cstdint> +#include <format> +#include <limits> +#include <regex> +#include <string> +#include <string_view> +#include <utility> +#include <vector> + +#include "iceberg/expression/binder.h" +#include "iceberg/expression/literal.h" +#include "iceberg/expression/predicate.h" +#include "iceberg/expression/term.h" +#include "iceberg/transform.h" +#include "iceberg/type.h" +#include "iceberg/util/bucket_util.h" +#include "iceberg/util/checked_cast.h" +#include "iceberg/util/temporal_util.h" + +namespace iceberg { + +namespace { + +std::string SanitizeDate(int32_t days, int32_t today) { + std::string is_past = today > days ? "ago" : "from-now"; + uint32_t diff = today > days ? static_cast<uint32_t>(static_cast<uint32_t>(today) - + static_cast<uint32_t>(days)) + : static_cast<uint32_t>(static_cast<uint32_t>(days) - + static_cast<uint32_t>(today)); + if (diff == 0) { + return "(date-today)"; + } else if (diff < 90) { + return "(date-" + std::to_string(diff) + "-days-" + is_past + ")"; + } + + return "(date)"; +} + +std::string SanitizeTimestamp(int64_t micros, int64_t now) { + constexpr int64_t kMicrosPerHour = 60LL * 60LL * 1'000'000LL; + constexpr int64_t kFiveMinutesInMicros = 5LL * 60LL * 1'000'000LL; + constexpr int64_t kThreeDaysInHours = 3LL * 24LL; + constexpr int64_t kNinetyDaysInHours = 90LL * 24LL; + + std::string is_past = now > micros ? "ago" : "from-now"; + uint64_t diff = now > micros ? static_cast<uint64_t>(static_cast<uint64_t>(now) - + static_cast<uint64_t>(micros)) + : static_cast<uint64_t>(static_cast<uint64_t>(micros) - + static_cast<uint64_t>(now)); + if (diff < kFiveMinutesInMicros) { + return "(timestamp-about-now)"; + } + + int64_t hours = diff / kMicrosPerHour; + if (hours <= kThreeDaysInHours) { + return "(timestamp-" + std::to_string(hours) + "-hours-" + is_past + ")"; + } else if (hours < kNinetyDaysInHours) { + int64_t days = hours / 24; + return "(timestamp-" + std::to_string(days) + "-days-" + is_past + ")"; + } + + return "(timestamp)"; +} + +std::string SanitizeNumber(double value, std::string_view type) { + int32_t num_digits = + value == 0 ? 1 : static_cast<int32_t>(std::log10(std::abs(value))) + 1; + return std::format("({}-digit-{})", num_digits, type); +} + +Result<std::string> SanitizeSimpleString(std::string_view value) { + ICEBERG_ASSIGN_OR_RAISE(auto hash, + BucketUtils::BucketIndex(Literal::String(std::string(value)), + std::numeric_limits<int32_t>::max())); + return std::format("(hash-{:08x})", hash); +} + +Result<std::string> SanitizeString(std::string_view value, int64_t now, int32_t today) { + static const std::regex kDate(R"(\d{4}-\d{2}-\d{2})"); + static const std::regex kTime(R"(\d{2}:\d{2}(:\d{2}(.\d{1,9})?)?)"); + static const std::regex kTimestamp( + R"(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2}(.\d{1,9})?)?)"); + static const std::regex kTimestampTz( + R"(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2}(.\d{1,9})?)?([-+]\d{2}:\d{2}|Z))"); + static const std::regex kTimestampNs( + R"(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2}(.\d{7,9})?)?)"); + static const std::regex kTimestampTzNs( + R"(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2}(.\d{7,9})?)?([-+]\d{2}:\d{2}|Z))"); Review Comment: The time/timestamp regexes use an unescaped '.' inside the fractional-seconds group (e.g. '(.d{1,9})'), which matches any character rather than a literal dot. This makes the classification overly permissive and can mis-sanitize strings that aren't actually timestamps. Escape '.' as '\\.' in these patterns. ########## src/iceberg/test/rest_metrics_reporter_test.cc: ########## @@ -0,0 +1,158 @@ +/* + * 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/catalog/rest/rest_metrics_reporter.h" + +#include <memory> +#include <string> +#include <unordered_map> +#include <utility> +#include <vector> + +#include <gmock/gmock.h> +#include <gtest/gtest.h> +#include <nlohmann/json.hpp> + +#include "iceberg/catalog/rest/auth/auth_session.h" +#include "iceberg/catalog/rest/error_handlers.h" +#include "iceberg/catalog/rest/http_client.h" +#include "iceberg/metrics/commit_report.h" +#include "iceberg/metrics/metrics_reporter.h" +#include "iceberg/metrics/scan_report.h" +#include "iceberg/result.h" +#include "iceberg/test/matchers.h" + +namespace iceberg::rest { + +namespace { + +// A minimal HttpClientBase test double: RestMetricsReporter only ever calls Post(), so +// only that method needs to be mockable (see the migration comment on HttpClientBase). +class MockHttpClient : public HttpClientBase { + public: + MOCK_METHOD(Result<HttpResponse>, Post, + (const std::string& path, const std::string& body, + (const std::unordered_map<std::string, std::string>& headers), + const ErrorHandler& error_handler, auth::AuthSession& session), + (override)); +}; + +} // namespace + +class RestMetricsReporterTest : public ::testing::Test { + protected: + void SetUp() override { + client_ = std::make_shared<HttpClient>(); + session_ = auth::AuthSession::MakeDefault({}); + } + + std::shared_ptr<HttpClient> client_; + std::shared_ptr<auth::AuthSession> session_; +}; + +namespace { + +// A Scan/Commit report test case: the report to send plus what a correct request for +// it must contain. Parameterizing over this collapses what would otherwise be 3 +// near-identical Scan/Commit test-body pairs into 3 bodies total. +struct ReportTestCase { + std::string name; // instantiation test-name suffix, e.g. "ScanReport" + MetricsReport report; + std::string expected_report_type; + std::vector<std::pair<std::string, nlohmann::json>> expected_fields; +}; + +ReportTestCase MakeScanReportCase() { + ScanReport report; + report.table_name = "ns.tbl"; + report.snapshot_id = 42; + report.schema_id = 0; + return {"ScanReport", + report, + "scan-report", + {{"table-name", "ns.tbl"}, {"snapshot-id", 42}}}; +} + +ReportTestCase MakeCommitReportCase() { + CommitReport report; + report.table_name = "ns.tbl"; + report.snapshot_id = 99; + report.sequence_number = 1; + report.operation = "append"; + return {"CommitReport", + report, + "commit-report", + {{"table-name", "ns.tbl"}, + {"snapshot-id", 99}, + {"sequence-number", 1}, + {"operation", "append"}}}; +} + +} // namespace + +class RestMetricsReporterPayloadTest + : public RestMetricsReporterTest, + public ::testing::WithParamInterface<ReportTestCase> {}; + +// Report() must return OK even when the HTTP call fails (connection refused). +// This validates the fire-and-forget error-suppression contract matching Java behavior. +TEST_P(RestMetricsReporterPayloadTest, ReportSuppressesHttpErrors) { + RestMetricsReporter reporter(client_, "http://localhost:0/v1/ns/tables/tbl/metrics";, + session_); + EXPECT_THAT(reporter.Report(GetParam().report), IsOk()); +} Review Comment: This test uses a real HttpClient and an endpoint that will attempt a real network connection ("localhost:0"). That can be flaky/slow in CI and isn't needed to validate the fire-and-forget error suppression contract. Prefer a MockHttpClient that returns an error Result from Post(). ########## src/iceberg/catalog/rest/rest_metrics_reporter.cc: ########## @@ -0,0 +1,80 @@ +/* + * 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/catalog/rest/rest_metrics_reporter.h" + +#include <cstdio> +#include <utility> + +#include <nlohmann/json.hpp> + +#include "iceberg/catalog/rest/auth/auth_session.h" +#include "iceberg/catalog/rest/error_handlers.h" +#include "iceberg/catalog/rest/http_client.h" +#include "iceberg/metrics/json_serde_internal.h" +#include "iceberg/metrics/metrics_reporter.h" + +namespace iceberg::rest { + +namespace { + +constexpr std::string_view kReportType = "report-type"; +constexpr std::string_view kScanReportType = "scan-report"; +constexpr std::string_view kCommitReportType = "commit-report"; + +} // namespace + +RestMetricsReporter::RestMetricsReporter(std::shared_ptr<HttpClientBase> client, + std::string metrics_endpoint, + std::shared_ptr<auth::AuthSession> session) + : client_(std::move(client)), + metrics_endpoint_(std::move(metrics_endpoint)), + session_(std::move(session)) {} + +Result<std::string> RestMetricsReporter::BuildRequestBody(const MetricsReport& report) { + ICEBERG_ASSIGN_OR_RAISE( + auto json, + std::visit([](const auto& r) -> Result<nlohmann::json> { return ToJson(r); }, + report)); + + // Inject "report-type" required by the REST spec (not included in core ToJson). + json[kReportType] = + std::holds_alternative<ScanReport>(report) ? kScanReportType : kCommitReportType; + return json.dump(); +} + +Status RestMetricsReporter::Report(const MetricsReport& report) { + try { + auto body_result = BuildRequestBody(report); + if (!body_result) { + return {}; + } Review Comment: RestMetricsReporter::Report() unconditionally dereferences client_ and session_. If either is null (misconfiguration, moved-from reporter, etc.), this will crash; since the contract is fire-and-forget with suppressed errors, a null-guard should return OK instead. ########## src/iceberg/table_scan.cc: ########## @@ -569,11 +598,53 @@ Result<std::vector<std::shared_ptr<FileScanTask>>> DataTableScan::PlanFiles() co .FilterData(filter()) .IgnoreDeleted() .ColumnsToKeepStats(context_.columns_to_keep_stats) - .PlanWith(context_.plan_executor); + .PlanWith(context_.plan_executor) + .ScanMetrics(scan_metrics); if (context_.ignore_residuals) { manifest_group->IgnoreResiduals(); } - return manifest_group->PlanFiles(); + ICEBERG_ASSIGN_OR_RAISE(auto tasks, manifest_group->PlanFiles()); + + timed.Stop(); + + if (context_.metrics_reporter) { + ICEBERG_ASSIGN_OR_RAISE(auto projected_schema, ResolveProjectedSchema()); + const auto& schema_ptr = projected_schema.get(); Review Comment: When metrics_reporter is set, ScanReport.table_name is populated from context_.table_name but there is no guard that it was actually set. This can produce reports with an empty table_name, despite ScanReport documenting it as a fully-qualified table name. Consider enforcing TableName() when MetricsReporter() is configured. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
