Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/IO/S3/URI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@
#include <boost/algorithm/string/case_conv.hpp>
#include <Poco/Util/AbstractConfiguration.h>


namespace DB
{

struct URIConverter
{
static void modifyURI(Poco::URI & uri, std::unordered_map<std::string, std::string> mapper)
static void modifyURI(Poco::URI & uri, std::unordered_map<std::string, std::string> mapper, bool enable_url_encoding)
{
Macros macros({{"bucket", uri.getHost()}});
uri = macros.expand(mapper[uri.getScheme()]).empty() ? uri : Poco::URI(macros.expand(mapper[uri.getScheme()]) + uri.getPathAndQuery());
uri = macros.expand(mapper[uri.getScheme()]).empty()
? uri
: Poco::URI(macros.expand(mapper[uri.getScheme()]) + uri.getPathAndQuery(), enable_url_encoding);
}
};

Expand All @@ -32,7 +33,7 @@ namespace ErrorCodes
namespace S3
{

URI::URI(const std::string & uri_, bool allow_archive_path_syntax)
URI::URI(const std::string & uri_, bool allow_archive_path_syntax, bool enable_url_encoding)
{
/// Case when bucket name represented in domain name of S3 URL.
/// E.g. (https://bucket-name.s3.region.amazonaws.com/key)
Expand All @@ -54,7 +55,7 @@ URI::URI(const std::string & uri_, bool allow_archive_path_syntax)
else
uri_str = uri_;

uri = Poco::URI(uri_str);
uri = Poco::URI(uri_str, enable_url_encoding);

std::unordered_map<std::string, std::string> mapper;
auto context = Context::getGlobalContextInstance();
Expand All @@ -76,7 +77,7 @@ URI::URI(const std::string & uri_, bool allow_archive_path_syntax)
}

if (!mapper.empty())
URIConverter::modifyURI(uri, mapper);
URIConverter::modifyURI(uri, mapper, enable_url_encoding);
}

storage_name = "S3";
Expand Down
2 changes: 1 addition & 1 deletion src/IO/S3/URI.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ struct URI
bool is_virtual_hosted_style;

URI() = default;
explicit URI(const std::string & uri_, bool allow_archive_path_syntax = false);
explicit URI(const std::string & uri_, bool allow_archive_path_syntax = false, bool enable_url_encoding = true);
void addRegionToURI(const std::string & region);

static void validateBucket(const std::string & bucket, const Poco::URI & uri);
Expand Down
14 changes: 13 additions & 1 deletion src/Processors/Formats/Impl/AvroRowInputFormat.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ static void insertNumber(IColumn & column, WhichDataType type, T value)
case TypeIndex::DateTime64:
assert_cast<ColumnDecimal<DateTime64> &>(column).insertValue(static_cast<Int64>(value));
break;
case TypeIndex::Time64:
assert_cast<ColumnDecimal<Time64> &>(column).insertValue(static_cast<Int64>(value));
break;
case TypeIndex::IPv4:
assert_cast<ColumnIPv4 &>(column).insertValue(IPv4(static_cast<UInt32>(value)));
break;
Expand Down Expand Up @@ -303,6 +306,8 @@ AvroDeserializer::DeserializeFn AvroDeserializer::createDeserializeFn(const avro
return createDecimalDeserializeFn<DataTypeDecimal256>(root_node, target_type, false);
if (target.isDateTime64())
return createDecimalDeserializeFn<DataTypeDateTime64>(root_node, target_type, false);
if (target.isTime64())
return createDecimalDeserializeFn<DataTypeTime64>(root_node, target_type, false);
break;
case avro::AVRO_INT:
if (target_type->isValueRepresentedByNumber())
Expand Down Expand Up @@ -1282,8 +1287,11 @@ DataTypePtr AvroSchemaReader::avroNodeToDataType(avro::NodePtr node)
{
case avro::Type::AVRO_INT:
{
if (node->logicalType().type() == avro::LogicalType::DATE)
auto logical_type = node->logicalType();
if (logical_type.type() == avro::LogicalType::DATE)
return {std::make_shared<DataTypeDate32>()};
if (logical_type.type() == avro::LogicalType::TIME_MILLIS)
return {std::make_shared<DataTypeTime64>(3)};

return {std::make_shared<DataTypeInt32>()};
}
Expand All @@ -1294,6 +1302,10 @@ DataTypePtr AvroSchemaReader::avroNodeToDataType(avro::NodePtr node)
return {std::make_shared<DataTypeDateTime64>(3)};
if (logical_type.type() == avro::LogicalType::TIMESTAMP_MICROS)
return {std::make_shared<DataTypeDateTime64>(6)};
if (logical_type.type() == avro::LogicalType::TIME_MILLIS)
return {std::make_shared<DataTypeTime64>(3)};
if (logical_type.type() == avro::LogicalType::TIME_MICROS)
return {std::make_shared<DataTypeTime64>(6)};

return std::make_shared<DataTypeInt64>();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ bool canDumpIcebergStats(const Field & field, DataTypePtr type)
case TypeIndex::Date32:
case TypeIndex::Int64:
case TypeIndex::DateTime64:
case TypeIndex::Time:
case TypeIndex::Time64:
case TypeIndex::String:
return true;
default:
Expand All @@ -143,7 +145,9 @@ std::vector<uint8_t> dumpFieldToBytes(const Field & field, DataTypePtr type)
case TypeIndex::Date32:
return dumpValue(field.safeGet<Int32>());
case TypeIndex::Int64:
case TypeIndex::Time:
return dumpValue(field.safeGet<Int64>());
Comment on lines +148 to 149

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Serialize Time64 stats via decimal payload

TypeIndex::Time64 values are stored as decimal fields, but this branch reads them with field.safeGet<Int64>(). Since canDumpIcebergStats() now explicitly allows Time64, any Iceberg write that emits stats for a Time64 column can fail at runtime with a field type mismatch when building lower/upper bounds. This should use the same decimal extraction path as DateTime64 (or an equivalent conversion) instead of the integer accessor.

Useful? React with 👍 / 👎.

case TypeIndex::Time64:
case TypeIndex::DateTime64:
return dumpValue(field.safeGet<Decimal64>().getValue().value);
Comment on lines +148 to 152

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Serialize Time stats using Int64 field type

canDumpIcebergStats now allows TypeIndex::Time, but dumpFieldToBytes handles that branch with field.safeGet<Decimal64>(); Time values are stored in Field as Int64, so this path throws BAD_GET when lower/upper bounds are written. In practice, inserts into Iceberg tables that include a Time column can now fail during manifest statistics serialization instead of just skipping stats for that type.

Useful? React with 👍 / 👎.

Comment on lines +148 to 152

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Serialize Iceberg time bounds in microseconds

dumpFieldToBytes now emits raw ClickHouse storage units for Time/Time64 into manifest lower_bounds/upper_bounds, but Iceberg time bounds are defined as microseconds-from-midnight. Here Time is written as integer seconds and Time64 is written as raw decimal ticks, so values are mis-scaled unless they are already microseconds; e.g. Time and Time64(3) produce smaller bounds by 1e6/1e3. Any reader that trusts bounds for predicate pruning can incorrectly skip matching files and return incomplete results. Please rescale to microseconds (or skip writing bounds for non-microsecond scales) before serializing.

Useful? React with 👍 / 👎.

case TypeIndex::String:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ DataTypePtr IcebergSchemaProcessor::getSimpleType(const String & type_name, Cont
if (type_name == f_date)
return std::make_shared<DataTypeDate>();
if (type_name == f_time)
return std::make_shared<DataTypeInt64>();
return std::make_shared<DataTypeTime64>(6);
if (type_name == f_timestamp)
return std::make_shared<DataTypeDateTime64>(6);
if (type_name == f_timestamptz)
Expand Down
1 change: 1 addition & 0 deletions src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ std::pair<Poco::Dynamic::Var, bool> getIcebergType(DataTypePtr type, Int32 & ite
case TypeIndex::DateTime64:
return {"timestamp", true};
case TypeIndex::Time:
case TypeIndex::Time64:
return {"time", true};
case TypeIndex::String:
return {"string", true};
Expand Down
5 changes: 3 additions & 2 deletions src/Storages/ObjectStorage/Utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,8 @@ std::pair<DB::ObjectStoragePtr, std::string> resolveObjectStorageForPath(
{
normalized_path = "s3://" + target_decomposed.authority + "/" + target_decomposed.key;
}
S3::URI s3_uri(normalized_path);
// enable_url_encoding=false, path from metadata must have correct encoding already
S3::URI s3_uri(normalized_path, /*allow_archive_path_syntax*/ false, /*enable_url_encoding*/ false);

std::string key_to_use = s3_uri.key;

Expand All @@ -365,7 +366,7 @@ std::pair<DB::ObjectStoragePtr, std::string> resolveObjectStorageForPath(
{
normalized_table_location = "s3://" + table_location_decomposed.authority + "/" + table_location_decomposed.key;
}
S3::URI base_s3_uri(normalized_table_location);
S3::URI base_s3_uri(normalized_table_location, /*allow_archive_path_syntax*/ false, /*enable_url_encoding*/ false);

if (s3URIMatches(s3_uri, base_s3_uri.bucket, base_s3_uri.endpoint, target_scheme_normalized))
use_base_storage = true;
Expand Down
97 changes: 95 additions & 2 deletions tests/integration/test_database_iceberg/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import random
import time
import uuid
from datetime import datetime, timedelta
from datetime import datetime, timedelta, time as dtime

import pyarrow as pa
import pytest
Expand All @@ -26,7 +26,8 @@
StringType,
StructType,
TimestampType,
TimestamptzType
TimestamptzType,
TimeType,
)
from pyiceberg.table.sorting import UNSORTED_SORT_ORDER

Expand Down Expand Up @@ -939,3 +940,95 @@ def test_cluster_select(started_cluster):
assert len(cluster_secondary_queries) == 1

assert node2.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`", settings={"parallel_replicas_for_cluster_engines":1, 'enable_parallel_replicas': 2, 'cluster_for_parallel_replicas': 'cluster_simple', 'parallel_replicas_for_cluster_engines' : 1}) == 'pablo\n'


@pytest.mark.parametrize("storage_type", ["s3"])
def test_partitioning_by_time(started_cluster, storage_type):
node = started_cluster.instances["node1"]

test_ref = f"test_partitioning_by_time_{uuid.uuid4()}"
table_name = f"{test_ref}_table"
root_namespace = f"{test_ref}_namespace"

namespace = f"{root_namespace}.A"
catalog = load_catalog_impl(started_cluster)
catalog.create_namespace(namespace)

schema = Schema(
NestedField(
field_id=1,
name="key",
field_type=TimeType(),
required=False
),
NestedField(
field_id=2,
name="value",
field_type=StringType(),
required=False,
),
)

partition_spec = PartitionSpec(
PartitionField(
source_id=1, field_id=1000, transform=IdentityTransform(), name="partition_key"
)
)

table = create_table(catalog, namespace, table_name, schema=schema, partition_spec=partition_spec)
data = [{"key": dtime(12,0,0), "value": "test"}]
df = pa.Table.from_pylist(data)
table.append(df)

create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME)

assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}`") == "12:00:00.000000\ttest\n"


@pytest.mark.parametrize("storage_type", ["s3"])
def test_partitioning_by_string(started_cluster, storage_type):
node = started_cluster.instances["node1"]

test_ref = f"test_partitioning_by_string_{uuid.uuid4()}"
table_name = f"{test_ref}_table"
root_namespace = f"{test_ref}_namespace"

namespace = f"{root_namespace}.A"
catalog = load_catalog_impl(started_cluster)
catalog.create_namespace(namespace)

schema = Schema(
NestedField(
field_id=1,
name="key",
field_type=StringType(),
required=False
),
NestedField(
field_id=2,
name="value",
field_type=StringType(),
required=False,
),
NestedField(
field_id=3,
name="time_value",
field_type=TimeType(),
required=False,
),
)

partition_spec = PartitionSpec(
PartitionField(
source_id=1, field_id=1000, transform=IdentityTransform(), name="partition_key"
)
)

table = create_table(catalog, namespace, table_name, schema=schema, partition_spec=partition_spec)
data = [{"key": "a:b,c[d=e/f%g?h", "value": "test", "time_value": dtime(12,0,0)}]
df = pa.Table.from_pylist(data)
table.append(df)

create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME)

assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}`") == "a:b,c[d=e/f%g?h\ttest\t12:00:00.000000\n"
Loading