lxy-9602 commented on code in PR #479:
URL: https://github.com/apache/paimon-cpp/pull/479#discussion_r4412476127
##########
src/paimon/core/utils/nested_projection_utils.h:
##########
@@ -128,6 +128,15 @@ class PAIMON_EXPORT NestedProjectionUtils {
const std::shared_ptr<arrow::Array>& array,
const std::shared_ptr<arrow::DataType>& read_type, arrow::MemoryPool*
pool);
+ /// Relabel a TIMESTAMP in `data` with the zone `read_type` asks for, only
when both sides
+ /// have a zone and the same unit. A zoned value is an instant since the
epoch, so only the
+ /// type changes. STRUCT children are matched by name and
LIST/FIXED_SIZE_LIST elements are
+ /// followed; every other difference, a unit change included, is left as
is.
+ /// @return `data` itself when no TIMESTAMP is relabelled, otherwise a
shallow copy.
+ static std::shared_ptr<arrow::ArrayData> AlignTimestampTimezones(
+ const std::shared_ptr<arrow::ArrayData>& data,
+ const std::shared_ptr<arrow::DataType>& read_type);
+
Review Comment:
This function seems to mainly handle `arrow::Array`. I suggest modify `data`
to `array` to reduce low-level dependencies and to keep it consistent with
other functions of this class.
##########
test/test_data/lance/append_ltz_python_compat.db/append_ltz_python_compat/README.md:
##########
@@ -0,0 +1,62 @@
+<!--
+ 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.
+-->
Review Comment:
Other READMEs don’t include a License section—please keep it consistent.
##########
test/inte/write_and_read_inte_test.cpp:
##########
@@ -6027,6 +6028,115 @@ TEST(TimeColumnWriteInteTest,
TestAppendTimeColumnFinalize) {
}
#endif
+#ifdef PAIMON_ENABLE_LANCE
+class LanceTimestampLtzInteTest
+ : public ::testing::Test,
+ public ::testing::WithParamInterface<std::tuple<std::string, bool>> {};
+
+TEST_P(LanceTimestampLtzInteTest, TestWriteAndReadInPinnedTimezone) {
+ TimezoneGuard timezone_guard(std::get<0>(GetParam()));
+ const std::string timezone = DateTimeUtils::GetLocalTimezoneName();
+ const bool with_primary_key = std::get<1>(GetParam());
+ auto dir = UniqueTestDirectory::Create("local");
+ ASSERT_NE(dir, nullptr);
+
+ auto ltz = [&timezone](arrow::TimeUnit::type unit) { return
arrow::timestamp(unit, timezone); };
+ arrow::FieldVector fields = {
+ arrow::field("id", arrow::int32()),
+ arrow::field("ltz_0", ltz(arrow::TimeUnit::SECOND)),
+ arrow::field("ltz_9", ltz(arrow::TimeUnit::NANO)),
+ arrow::field("ltz_array", arrow::list(ltz(arrow::TimeUnit::MICRO))),
+ arrow::field(
+ "ltz_row",
+ arrow::struct_({arrow::field("ltz_3", ltz(arrow::TimeUnit::MILLI)),
+ arrow::field("ntz",
arrow::timestamp(arrow::TimeUnit::MICRO))})),
+ };
+ std::map<std::string, std::string> options = {{Options::FILE_FORMAT,
"lance"},
+ {Options::BUCKET,
with_primary_key ? "1" : "-1"},
+ {Options::FILE_SYSTEM,
"local"}};
+ std::vector<std::string> primary_keys;
+ if (with_primary_key) {
+ primary_keys.push_back("id");
+ }
+ ASSERT_OK_AND_ASSIGN(std::unique_ptr<TestHelper> helper,
+ TestHelper::Create(dir->Str(), arrow::schema(fields),
+ /*partition_keys=*/{},
primary_keys, options,
+ /*is_streaming_mode=*/false));
+ ASSERT_OK_AND_ASSIGN(std::unique_ptr<RecordBatch> batch_v0,
+ TestHelper::MakeRecordBatch(arrow::struct_(fields),
R"([
+ [1, "1969-12-31 23:59:59", "1969-12-31 23:59:59.999999999",
+ ["2024-10-04 12:34:56.123456", null],
+ ["1970-01-01 00:00:00.001", "1970-01-01 00:00:00.000001"]],
+ [2, null, null, null, [null, null]],
+ [3, "2024-10-04 12:34:56", "2024-10-04 12:34:56.123456789", [],
+ ["2024-10-04 12:34:56.123", null]]
+ ])",
+ /*partition_map=*/{},
/*bucket=*/0, {}));
+ ASSERT_OK(helper->WriteAndCommit(std::move(batch_v0),
/*commit_identifier=*/0,
+
/*expected_commit_messages=*/std::nullopt));
+
+ const std::string table_path = PathUtil::JoinPath(dir->Str(),
"foo.db/bar");
+ SchemaManager schema_manager(dir->GetFileSystem(), table_path);
+ ASSERT_OK_AND_ASSIGN(std::shared_ptr<TableSchema> schema_v0,
schema_manager.ReadSchema(0));
+ const int32_t added_field_id = schema_v0->HighestFieldId() + 1;
+ std::shared_ptr<arrow::Field> added_field =
+ arrow::field("ltz_added", ltz(arrow::TimeUnit::MICRO));
+ std::vector<DataField> data_fields_v1 = schema_v0->Fields();
+ data_fields_v1.emplace_back(added_field_id, added_field);
+ ASSERT_OK(TestHelper::WriteNextSchema(dir->GetFileSystem(), table_path,
data_fields_v1,
+ added_field_id, options));
+ arrow::FieldVector fields_v1 = fields;
Review Comment:
Thanks for the thorough testing, especially for including schema evolution
coverage!
Could you also extend `TestWriteAndReadInPinnedTimezone` to cover changing
an existing column from `TIMESTAMP` to `TIMESTAMP_LTZ` and vice versa? Please
include a non-UTC time zone (may use `TimezoneGuard`) and verify that reading
files written before the type change produces the same values as Paimon Java
under the same default time zone. Java applies a time zone conversion in these
cases, so this would help confirm that the schema evolution behavior is
consistent across both implementations.
##########
src/paimon/core/utils/nested_projection_utils.cpp:
##########
@@ -840,4 +840,65 @@ Result<std::shared_ptr<arrow::Array>>
NestedProjectionUtils::AlignArrayToReadTyp
}
}
+std::shared_ptr<arrow::ArrayData>
NestedProjectionUtils::AlignTimestampTimezones(
+ const std::shared_ptr<arrow::ArrayData>& data,
+ const std::shared_ptr<arrow::DataType>& read_type) {
+ const std::shared_ptr<arrow::DataType>& type = data->type;
+ if (type->id() == arrow::Type::TIMESTAMP && read_type->id() ==
arrow::Type::TIMESTAMP) {
+ const auto& stored = checked_cast<const arrow::TimestampType&>(*type);
+ const auto& wanted = checked_cast<const
arrow::TimestampType&>(*read_type);
+ if (stored.unit() != wanted.unit() || stored.timezone().empty() ||
+ wanted.timezone().empty() || stored.timezone() ==
wanted.timezone()) {
+ return data;
+ }
+ std::shared_ptr<arrow::ArrayData> retyped = data->Copy();
+ retyped->type = arrow::timestamp(wanted.unit(), wanted.timezone());
+ return retyped;
+ }
+ if (type->id() == arrow::Type::STRUCT && read_type->id() ==
arrow::Type::STRUCT) {
+ const auto& wanted_struct = checked_cast<const
arrow::StructType&>(*read_type);
+ arrow::FieldVector fields = type->fields();
+ std::shared_ptr<arrow::ArrayData> retyped;
+ for (int32_t i = 0; i < static_cast<int32_t>(data->child_data.size());
++i) {
+ const int32_t index =
wanted_struct.GetFieldIndex(fields[i]->name());
+ if (index < 0) {
+ continue;
+ }
+ std::shared_ptr<arrow::ArrayData> child =
+ AlignTimestampTimezones(data->child_data[i],
wanted_struct.field(index)->type());
+ if (child == data->child_data[i]) {
+ continue;
+ }
+ if (retyped == nullptr) {
+ retyped = data->Copy();
+ }
+ retyped->child_data[i] = child;
+ fields[i] = fields[i]->WithType(child->type);
+ }
+ if (retyped == nullptr) {
+ return data;
+ }
+ retyped->type = arrow::struct_(fields);
+ return retyped;
+ }
+ if ((type->id() == arrow::Type::LIST || type->id() ==
arrow::Type::FIXED_SIZE_LIST) &&
+ type->id() == read_type->id() && !data->child_data.empty()) {
+ std::shared_ptr<arrow::ArrayData> values =
+ AlignTimestampTimezones(data->child_data[0],
read_type->field(0)->type());
+ if (values == data->child_data[0]) {
+ return data;
+ }
+ std::shared_ptr<arrow::ArrayData> retyped = data->Copy();
+ retyped->child_data[0] = values;
+ const std::shared_ptr<arrow::Field> value_field =
type->field(0)->WithType(values->type);
+ retyped->type = type->id() == arrow::Type::LIST
+ ? arrow::list(value_field)
+ : arrow::fixed_size_list(
+ value_field,
+ checked_cast<const
arrow::FixedSizeListType&>(*type).list_size());
+ return retyped;
+ }
+ return data;
Review Comment:
I understand that Lance may not support map types, but since this is a utils
function, please add handling for the map branch as well.
--
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]
